Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
488433003:
Remove unused enum declaration ContextWebkit3d from HTMLCanvasElement::getContext() (Closed)
|
Created:
6 years, 4 months ago by vivekg_samsung Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionRemove unused enum declaration ContextWebkit3d from HTMLCanvasElement::getContext()
webkit-3d is no longer used in getContext and hence should be removed.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180853
Patch Set 1 #
Total comments: 3
Patch Set 2 : Patch for landing! #
Total comments: 3
Patch Set 3 : #Messages
Total messages: 14 (0 generated)
PTAL, thanks!
https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (left): https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:159: // Only add new items to the end and keep the order of existing items. This is not my area of expertise, so you should try to find someone who has. However, the comment above indicates that the integer values for the enums above is important, so removing one of the enums might break something.
https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (left): https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:159: // Only add new items to the end and keep the order of existing items. On 2014/08/19 08:47:49, rune wrote: > > This is not my area of expertise, so you should try to find someone who has. > However, the comment above indicates that the integer values for the enums above > is important, so removing one of the enums might break something. Looking further down the code, the integer value of the enum is used for histograms, so not lgtm.
https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (left): https://codereview.chromium.org/488433003/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:159: // Only add new items to the end and keep the order of existing items. On 2014/08/19 08:52:03, rune wrote: > On 2014/08/19 08:47:49, rune wrote: > > > > This is not my area of expertise, so you should try to find someone who has. > > However, the comment above indicates that the integer values for the enums > above > > is important, so removing one of the enums might break something. > > Looking further down the code, the integer value of the enum is used for > histograms, so not lgtm. Thank you for the review @rune. Agreed that the total number of enums i.e. ContextTypeCount value is used for the histograms. I am not sure though whether this would break anything specifically. Also all the other enums values are used for comparisons but ContextWebkit3d. Not sure its worth keeping it here. I am also adding @junov for the feedback.
+senorblanco
In other parts of Blink where enums are used for histograms, we started explicitly assigning integer values to avoid accidents. You could do that here, and beef-up the comment to explain that the enum values must be preserved (not just the order). That way it is safe to remove obsolete entries.
On 2014/08/21 at 18:39:07, junov wrote: > In other parts of Blink where enums are used for histograms, we started explicitly assigning integer values to avoid accidents. You could do that here, and beef-up the comment to explain that the enum values must be preserved (not just the order). That way it is safe to remove obsolete entries. Done. PTAL, thank you!
https://codereview.chromium.org/488433003/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/488433003/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:157: ContextExperimentalWebgl = 2, I think it would be safer to explicitly assign a value to each enum. See how it's done for the Feature enum in the UseCounter class. https://codereview.chromium.org/488433003/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:157: ContextExperimentalWebgl = 2, I think it would be safer to explicitly assign a value to each enum. See how it's done for the Feature enum in the UseCounter class. https://codereview.chromium.org/488433003/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:159: // Only add new items to the end and keep the order of existing items. The comment in UseCounter for Feature looks clearer: // Do not change assigned numbers of existing items: add new features // to the end of the list. Please consider using that comment and move it to the top of the enum.
Done, thank you! PTAL.
lgtm
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/488433003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24207)
Message was sent while issue was closed.
Committed patchset #3 (40001) as 180853 |
