Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799213003:
Remove deprecated functions from Canvas in favor of the RectF version. (Closed)
DescriptionRemove deprecated functions from Canvas in favor of the RectF version.
BUG=553726
Review-Url: https://codereview.chromium.org/2799213003
Cr-Commit-Position: refs/heads/master@{#463518}
Committed: https://chromium.googlesource.com/chromium/src/+/81137314dd619ccb9b3ca3475616a47447a99c58
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed compile error in content_analysis_unittest #
Total comments: 4
Patch Set 3 : Addressed review comments #
Total comments: 2
Patch Set 4 : Remove deprecated functions from Canvas in favor of the RectF version #
Messages
Total messages: 36 (16 generated)
Description was changed from ========== Remove deprecated functions from canvas BUG=None ========== to ========== Remove deprecated functions from canvas BUG=None ==========
uzair.jaleel@samsung.com changed reviewers: + mgiuca@chromium.org
On 2017/04/06 13:17:52, Uzair wrote: > mailto:uzair.jaleel@samsung.com changed reviewers: > + mailto:mgiuca@chromium.org Dear Matt, PTAL. If the change is fine there are many other similar functions i will upload another patch for the same. Thanks
Description was changed from ========== Remove deprecated functions from canvas BUG=None ========== to ========== Remove deprecated functions from canvas in favor of the RectF version. BUG=None ==========
uzair.jaleel@samsung.com changed reviewers: + a.suchit2@chromium.org
uzair.jaleel@samsung.com changed reviewers: + a.suchit@chromium.org - a.suchit2@chromium.org
uzair.jaleel@samsung.com changed reviewers: + msw@chromium.org
Hi, this lgtm. Please update the CL description: - Capitalize 'C' in Canvas so it's clear you refer to the class name. - Add BUG=553726 (the original bug that resulted in creating the float versions and deprecating the int versions). You will also have to wait for an lg from msw@.
I checked all the other usages of deprecated methods in this class, and they all have at least one usage. (Except for the one I noted in the review comments, which I think should be deleted in this CL.) Ideally we'd clean up all the other usages too, but I'm happy to just land this, removing the ones that aren't in use but keeping the others as deprecated. https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (left): https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h#oldcode244 ui/gfx/canvas.h:244: void DrawRect(const Rect& rect, SkColor color); This function seems to still be used by chrome/browser/thumbnails/content_analysis_unittest.cc. You will have to replace those usages with the RectF version before this can land. https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h#newcode245 ui/gfx/canvas.h:245: void DrawRect(const Rect& rect, SkColor color, SkBlendMode mode); This one doesn't seem to have any uses so can be removed. Do that here.
Description was changed from ========== Remove deprecated functions from canvas in favor of the RectF version. BUG=None ========== to ========== Remove deprecated functions from canvas in favor of the RectF version. BUG=553726 ==========
Description was changed from ========== Remove deprecated functions from canvas in favor of the RectF version. BUG=553726 ========== to ========== Remove deprecated functions from Canvas in favor of the RectF version. BUG=553726 ==========
Done https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (left): https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h#oldcode244 ui/gfx/canvas.h:244: void DrawRect(const Rect& rect, SkColor color); On 2017/04/07 03:47:54, Matt Giuca wrote: > This function seems to still be used by > chrome/browser/thumbnails/content_analysis_unittest.cc. > > You will have to replace those usages with the RectF version before this can > land. Done. https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2799213003/diff/1/ui/gfx/canvas.h#newcode245 ui/gfx/canvas.h:245: void DrawRect(const Rect& rect, SkColor color, SkBlendMode mode); On 2017/04/07 03:47:54, Matt Giuca wrote: > This one doesn't seem to have any uses so can be removed. Do that here. Done.
https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... File chrome/browser/thumbnails/content_analysis_unittest.cc (right): https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:132: canvas.DrawRect(gfx::RectF(draw_rect), SkColorSetRGB(255, 255, 255)); Don't cast the Rect to a RectF. Just change the type of the |draw_rect| variable to RectF. Also you need to #include "ui/gfx/geometry/rect_f.h" https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:652: canvas.DrawRect(gfx::RecFt(x, y, fine_print, fine_print), print_color); RectF
Done https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... File chrome/browser/thumbnails/content_analysis_unittest.cc (right): https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:132: canvas.DrawRect(gfx::RectF(draw_rect), SkColorSetRGB(255, 255, 255)); On 2017/04/07 06:38:08, Matt Giuca wrote: > Don't cast the Rect to a RectF. Just change the type of the |draw_rect| variable > to RectF. > > Also you need to #include "ui/gfx/geometry/rect_f.h" Done. https://codereview.chromium.org/2799213003/diff/20001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:652: canvas.DrawRect(gfx::RecFt(x, y, fine_print, fine_print), print_color); On 2017/04/07 06:38:08, Matt Giuca wrote: > RectF Apologies for the mistake.
Nice, lgtm with a cautionary comment/q (probably fine as-is). https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... File chrome/browser/thumbnails/content_analysis_unittest.cc (right): https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:663: gfx::RectF pict_rect(x, y, body_rect.width() / 2 - 2 * margin_vertical, Should these div ops use floating point literals now? (probably doesn't matter for this test, just be careful with this and check for implicit and explicit floor/ceil/round operations in other int->float conversion work).
PTAL https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... File chrome/browser/thumbnails/content_analysis_unittest.cc (right): https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... chrome/browser/thumbnails/content_analysis_unittest.cc:663: gfx::RectF pict_rect(x, y, body_rect.width() / 2 - 2 * margin_vertical, On 2017/04/07 10:12:23, msw wrote: > Should these div ops use floating point literals now? (probably doesn't matter > for this test, just be careful with this and check for implicit and explicit > floor/ceil/round operations in other int->float conversion work). How about canvas.drawRect(gfx::RectF(pict_rect),..,..) ?
Message was sent while issue was closed.
On 2017/04/07 10:53:55, Uzair wrote: > PTAL > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > File chrome/browser/thumbnails/content_analysis_unittest.cc (right): > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > chrome/browser/thumbnails/content_analysis_unittest.cc:663: gfx::RectF > pict_rect(x, y, body_rect.width() / 2 - 2 * margin_vertical, > On 2017/04/07 10:12:23, msw wrote: > > Should these div ops use floating point literals now? (probably doesn't matter > > for this test, just be careful with this and check for implicit and explicit > > floor/ceil/round operations in other int->float conversion work). > > How about canvas.drawRect(gfx::RectF(pict_rect),..,..) ? Hi , This issue has been closed by mistake. Is there any way to re-open it. Thanks
On 2017/04/07 11:03:01, Uzair wrote: > On 2017/04/07 10:53:55, Uzair wrote: > > PTAL > > > > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > > File chrome/browser/thumbnails/content_analysis_unittest.cc (right): > > > > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > > chrome/browser/thumbnails/content_analysis_unittest.cc:663: gfx::RectF > > pict_rect(x, y, body_rect.width() / 2 - 2 * margin_vertical, > > On 2017/04/07 10:12:23, msw wrote: > > > Should these div ops use floating point literals now? (probably doesn't > matter > > > for this test, just be careful with this and check for implicit and explicit > > > floor/ceil/round operations in other int->float conversion work). > > > > How about canvas.drawRect(gfx::RectF(pict_rect),..,..) ? > > Hi , > > This issue has been closed by mistake. > Is there any way to re-open it. > > Thanks Sorry issue is open now Dear Ms PTAL THANKS
On 2017/04/07 16:10:10, Uzair wrote: > On 2017/04/07 11:03:01, Uzair wrote: > > On 2017/04/07 10:53:55, Uzair wrote: > > > PTAL > > > > > > > > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > > > File chrome/browser/thumbnails/content_analysis_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2799213003/diff/40001/chrome/browser/thumbnai... > > > chrome/browser/thumbnails/content_analysis_unittest.cc:663: gfx::RectF > > > pict_rect(x, y, body_rect.width() / 2 - 2 * margin_vertical, > > > On 2017/04/07 10:12:23, msw wrote: > > > > Should these div ops use floating point literals now? (probably doesn't > > matter > > > > for this test, just be careful with this and check for implicit and > explicit > > > > floor/ceil/round operations in other int->float conversion work). > > > > > > How about canvas.drawRect(gfx::RectF(pict_rect),..,..) ? > > > > Hi , > > > > This issue has been closed by mistake. > > Is there any way to re-open it. > > > > Thanks > > Sorry issue is open now Dear Msw PTAL Thanks
lgtm
The CQ bit was checked by uzair.jaleel@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2799213003/#ps50001 (title: "Remove deprecated functions from Canvas in favor of the RectF version")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
uzair.jaleel@samsung.com changed reviewers: + thestig@chromium.org
On 2017/04/09 12:59:10, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Dear Reviewers, Need review for chrome/browser/thumbnails/content_analysis_unittest.cc Thanks
a.suchit@chromium.org changed reviewers: + danakj@chromium.org
On 2017/04/10 04:11:35, Uzair wrote: > Dear Reviewers, > > Need review for chrome/browser/thumbnails/content_analysis_unittest.cc > > Thanks I think your comment was directed at me, rather than all 5 reviewers? Please be specific next time. lgtm
On 2017/04/10 17:56:22, Lei Zhang wrote: > On 2017/04/10 04:11:35, Uzair wrote: > > Dear Reviewers, > > > > Need review for chrome/browser/thumbnails/content_analysis_unittest.cc > > > > Thanks > > I think your comment was directed at me, rather than all 5 reviewers? Please be > specific next time. > > lgtm Yes thank you
The CQ bit was checked by uzair.jaleel@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 50001, "attempt_start_ts": 1491872487326380,
"parent_rev": "4cb31957c3d99982c42bdad33cdccd97e6709021", "commit_rev":
"81137314dd619ccb9b3ca3475616a47447a99c58"}
Message was sent while issue was closed.
Description was changed from ========== Remove deprecated functions from Canvas in favor of the RectF version. BUG=553726 ========== to ========== Remove deprecated functions from Canvas in favor of the RectF version. BUG=553726 Review-Url: https://codereview.chromium.org/2799213003 Cr-Commit-Position: refs/heads/master@{#463518} Committed: https://chromium.googlesource.com/chromium/src/+/81137314dd619ccb9b3ca3475616... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/81137314dd619ccb9b3ca3475616...
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
