OSSCA Chromium

기여 목록 / 8513859

Use a cast instead of @ts-expect-error in TimelineHistoryManager.onClick

DevToolsrefactordevtools/devtools-frontendMerged

panels/timeline에 마지막으로 남아 있던 crbug.com/1172300 주석을 제거한 컨트리뷰션입니다. 직접 찾은 이슈입니다.

문제 설명

  • DropDown.onClick이 event.target에 DevTools가 Element에 확장한 메서드를 호출
private onClick(event: Event): void {
  // TODO(crbug.com/1172300) Ignored during the jsdoc to ts migration
  // @ts-expect-error
  if (!(event.target).enclosingNodeOrSelfWithClass('preview-item')) {
  • Event.target의 타입은 EventTarget|null이고 해당 메서드가 없어 그대로는 컴파일되지 않음

해결 내용

(event.target as Node)로 캐스팅했습니다.

이슈를 올릴 때는 instanceof Element 가드가 필요하고 그러면 동작이 바뀐다고 적었는데, 같은 클래스의 onMouseMove를 보고 판단을 바꿨습니다.

private onMouseMove(event: Event): void {
  const node = (event.target as HTMLElement).enclosingNodeOrSelfWithClass('preview-item');
  • 9줄 위에서 같은 가정을 이미 캐스팅으로 처리하고 있었음
  • 두 핸들러 모두 같은 팝오버 내부 요소에 바인딩됨 (listControl.element, contentElement)
  • 가드를 넣으면 지금은 없는 "조용히 return" 분기가 새로 생기므로 오히려 동작 변경

테스트 방법

  1. panels/timeline:timeline 및 :unittests 빌드
  2. TimelineHistoryManager.test.ts → 5/5 통과
  3. 동작 변경 없음

배운 점

  • 같은 파일 안에 이미 선례가 있는지 먼저 확인해야 함
    • 이슈 본문에 "가드가 필요하고 동작 변경에 해당한다"고 단정했는데, 바로 위 함수가 답이었음
    • 억제 주석만 보고 해결 방법을 정하면 파일의 기존 관례와 어긋난 변경을 제안하게 됨
  • 가드 추가가 항상 안전한 선택은 아님
    • 지금 코드가 던지는 에러를 조용히 삼키게 되면 문제를 가리는 쪽이 됨

참고 자료

  • third_party/devtools-frontend/src/front_end/panels/timeline/TimelineHistoryManager.ts

리뷰 대응

  • Jack Franklin 님 lgtm, Mathias Bynens 님 +1 및 CQ+2 로 머지
  • 처음 올린 패치셋은 as HTMLElement 였고 커밋 메시지의 근거도 틀렸음
    • 헬퍼는 Element 가 아니라 Node.prototype 에 설치되어 있음 (ui/dom_extension/DOMExtension.ts:84, legacy/legacy-defs.d.ts:77)
    • 즉 Text 노드 타깃에서도 지금 코드는 던지지 않음. 가드를 넣는 쪽이 동작 변경이었음
    • as Node 로 바꾸고 메시지도 수정
  • Mathias 님이 Monorail ID 대신 Buganizer ID 를 쓰라고 지적
    • Bug: 1172300 → Bug: 40166562
    • 커밋 메시지만 수정하면 Code-Review 표가 유지됨을 확인 (copy condition 에 NO_CODE_CHANGE 포함). 코드가 바뀌면 리셋됨
  • 미해결 코멘트가 남아 있으면 CQ+2 를 받아도 CV 가 시작되지 않음
    • Mathias 님이 자기 코멘트를 Done 으로 닫은 뒤에야 실행됨
    • 답변만으로는 해결되지 않고 unresolved: false 가 필요하며, 부모 코멘트와 같은 패치셋에 달아야 함
목록으로