Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613983002:
Rename paint related lifecycle methods of FrameView (Closed)
DescriptionRename paint related lifecycle methods of FrameView
updatePaintProperties -> prePaint
synchronizedPaint -> paint
synchronizedPaintRecursively -> paintGraphicsLayerRecursively
Review-Url: https://codereview.chromium.org/2613983002
Cr-Commit-Position: refs/heads/master@{#441953}
Committed: https://chromium.googlesource.com/chromium/src/+/7b6d0ee2e211c47f3fd5234d9cf005dcb748112f
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : Update comments #
Total comments: 2
Patch Set 4 : paintTree() and update android tools scripts #Patch Set 5 : Rebase on https://codereview.chromium.org/2615713004/ #
Depends on Patchset: Messages
Total messages: 42 (20 generated)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
On 2017/01/05 at 17:31:23, wangxianzhu wrote: > LGTM There are a few more comments with "synchronizedPaint" in them. Can you run "git grep -i synchronizedPaint" and rename them too?
On 2017/01/05 17:36:51, pdr. wrote: > On 2017/01/05 at 17:31:23, wangxianzhu wrote: > > > > LGTM > > There are a few more comments with "synchronizedPaint" in them. Can you run "git > grep -i synchronizedPaint" and rename them too? Done.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2613983002/#ps20001 (title: "-")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/05 at 17:42:00, wangxianzhu wrote: > On 2017/01/05 17:36:51, pdr. wrote: > > On 2017/01/05 at 17:31:23, wangxianzhu wrote: > > > > > > > LGTM > > > > There are a few more comments with "synchronizedPaint" in them. Can you run "git > > grep -i synchronizedPaint" and rename them too? > > Done. Just noticed there are a few comments with "updatePaintProperties" too (LayoutObject.h, FrameThrottlingTest).
The CQ bit was unchecked by wangxianzhu@chromium.org
On 2017/01/05 at 17:43:31, pdr. wrote: > On 2017/01/05 at 17:42:00, wangxianzhu wrote: > > On 2017/01/05 17:36:51, pdr. wrote: > > > On 2017/01/05 at 17:31:23, wangxianzhu wrote: > > > > > > > > > > LGTM > > > > > > There are a few more comments with "synchronizedPaint" in them. Can you run "git > > > grep -i synchronizedPaint" and rename them too? > > > > Done. > > Just noticed there are a few comments with "updatePaintProperties" too (LayoutObject.h, FrameThrottlingTest). There seems to be several more references to synchronizedPaintRecursively in tools/android/loading, something to do with metrics.
On 2017/01/05 17:43:31, pdr. wrote: > On 2017/01/05 at 17:42:00, wangxianzhu wrote: > > On 2017/01/05 17:36:51, pdr. wrote: > > > On 2017/01/05 at 17:31:23, wangxianzhu wrote: > > > > > > > > > > LGTM > > > > > > There are a few more comments with "synchronizedPaint" in them. Can you run > "git > > > grep -i synchronizedPaint" and rename them too? > > > > Done. > > Just noticed there are a few comments with "updatePaintProperties" too > (LayoutObject.h, FrameThrottlingTest). Done for LayoutObject.h (InUpdatePaintProperties -> InPrePaint). The one in FrameThrottlingTest should be fine (a test named UpdatePaintPrpertiesOnUnthrottling).
https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); With this CL there are now 3 methods called paint(). Could you you rename the other two to be more specific? Or rename this to paintAllContent or similar?
https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); On 2017/01/05 17:51:10, chrishtr wrote: > With this CL there are now 3 methods called paint(). Could you you rename > the other two to be more specific? Or rename this to paintAllContent or > similar? One of the other two is Widget override. paintAllContent seems ambiguous because the word "contents" in the existing paintContents() means things other than scrollbars. How about paintTree()?
On 2017/01/05 at 18:05:09, wangxianzhu wrote: > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > On 2017/01/05 17:51:10, chrishtr wrote: > > With this CL there are now 3 methods called paint(). Could you you rename > > the other two to be more specific? Or rename this to paintAllContent or > > similar? > > One of the other two is Widget override. The other one is for software paint for printing (or SPv2) right? > > paintAllContent seems ambiguous because the word "contents" in the existing paintContents() means things other than scrollbars. > > How about paintTree()? paintTree() works for me, and is similar to other lifecycle methods.
On 2017/01/05 18:05:09, Xianzhu wrote: > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > On 2017/01/05 17:51:10, chrishtr wrote: > > With this CL there are now 3 methods called paint(). Could you you rename > > the other two to be more specific? Or rename this to paintAllContent or > > similar? > > One of the other two is Widget override. > > paintAllContent seems ambiguous because the word "contents" in the existing > paintContents() means things other than scrollbars. > > How about paintTree()? Or just leave the overloads? For SPv2, FrameView::paint() will be just a simple wrapper of FrameView::paint(GraphicsContext&, ...).
On 2017/01/05 18:10:58, chrishtr wrote: > On 2017/01/05 at 18:05:09, wangxianzhu wrote: > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > > On 2017/01/05 17:51:10, chrishtr wrote: > > > With this CL there are now 3 methods called paint(). Could you you rename > > > the other two to be more specific? Or rename this to paintAllContent or > > > similar? > > > > One of the other two is Widget override. > > The other one is for software paint for printing (or SPv2) right? > > > > paintAllContent seems ambiguous because the word "contents" in the existing > paintContents() means things other than scrollbars. > > > > How about paintTree()? > > paintTree() works for me, and is similar to other lifecycle methods. Acknowledged.
On 2017/01/05 at 18:13:23, wangxianzhu wrote: > On 2017/01/05 18:10:58, chrishtr wrote: > > On 2017/01/05 at 18:05:09, wangxianzhu wrote: > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > > > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > > > On 2017/01/05 17:51:10, chrishtr wrote: > > > > With this CL there are now 3 methods called paint(). Could you you rename > > > > the other two to be more specific? Or rename this to paintAllContent or > > > > similar? > > > > > > One of the other two is Widget override. > > > > The other one is for software paint for printing (or SPv2) right? > > > > > > paintAllContent seems ambiguous because the word "contents" in the existing > > paintContents() means things other than scrollbars. > > > > > > How about paintTree()? > > > > paintTree() works for me, and is similar to other lifecycle methods. > > Acknowledged. Up to you, just document it if it's still overloaded.
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/05 17:44:33, pdr. wrote: > On 2017/01/05 at 17:43:31, pdr. wrote: > > On 2017/01/05 at 17:42:00, wangxianzhu wrote: > > > On 2017/01/05 17:36:51, pdr. wrote: > > > > On 2017/01/05 at 17:31:23, wangxianzhu wrote: > > > > > > > > > > > > > LGTM > > > > > > > > There are a few more comments with "synchronizedPaint" in them. Can you > run "git > > > > grep -i synchronizedPaint" and rename them too? > > > > > > Done. > > > > Just noticed there are a few comments with "updatePaintProperties" too > (LayoutObject.h, FrameThrottlingTest). > > There seems to be several more references to synchronizedPaintRecursively in > tools/android/loading, something to do with metrics. Good catch. I had grepped third_party/WebKit only. Otherwise I may break this that can be noticed after some time. Done.
On 2017/01/05 18:10:58, chrishtr wrote: > On 2017/01/05 at 18:05:09, wangxianzhu wrote: > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > > On 2017/01/05 17:51:10, chrishtr wrote: > > > With this CL there are now 3 methods called paint(). Could you you rename > > > the other two to be more specific? Or rename this to paintAllContent or > > > similar? > > > > One of the other two is Widget override. > > The other one is for software paint for printing (or SPv2) right? > > > > paintAllContent seems ambiguous because the word "contents" in the existing > paintContents() means things other than scrollbars. > > > > How about paintTree()? > > paintTree() works for me, and is similar to other lifecycle methods. Done.
wangxianzhu@chromium.org changed reviewers: + blundell@chromium.org, mattcary@chromium.org
+mattcary, blendell for tools/android/loading.
+mattcary, blendell for tools/android/loading.
On 2017/01/05 at 18:49:46, wangxianzhu wrote: > On 2017/01/05 18:10:58, chrishtr wrote: > > On 2017/01/05 at 18:05:09, wangxianzhu wrote: > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > > > > > > > https://codereview.chromium.org/2613983002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/frame/FrameView.h:874: void paint(); > > > On 2017/01/05 17:51:10, chrishtr wrote: > > > > With this CL there are now 3 methods called paint(). Could you you rename > > > > the other two to be more specific? Or rename this to paintAllContent or > > > > similar? > > > > > > One of the other two is Widget override. > > > > The other one is for software paint for printing (or SPv2) right? > > > > > > paintAllContent seems ambiguous because the word "contents" in the existing > > paintContents() means things other than scrollbars. > > > > > > How about paintTree()? > > > > paintTree() works for me, and is similar to other lifecycle methods. > > Done. still LGTM :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
blundell@chromium.org changed reviewers: + droger@chromium.org - blundell@chromium.org, mattcary@chromium.org
-> drogerfor //tools/android/loading OWNERS
lgtm
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2613983002/#ps80001 (title: "Rebase on https://codereview.chromium.org/2615713004/")
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": 80001, "attempt_start_ts": 1483722073873750,
"parent_rev": "afe4920774dfdecdc31e9505647faee2f9b12da6", "commit_rev":
"7b6d0ee2e211c47f3fd5234d9cf005dcb748112f"}
Message was sent while issue was closed.
Description was changed from ========== Rename paint related lifecycle methods of FrameView updatePaintProperties -> prePaint synchronizedPaint -> paint synchronizedPaintRecursively -> paintGraphicsLayerRecursively ========== to ========== Rename paint related lifecycle methods of FrameView updatePaintProperties -> prePaint synchronizedPaint -> paint synchronizedPaintRecursively -> paintGraphicsLayerRecursively Review-Url: https://codereview.chromium.org/2613983002 Cr-Commit-Position: refs/heads/master@{#441953} Committed: https://chromium.googlesource.com/chromium/src/+/7b6d0ee2e211c47f3fd5234d9cf0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7b6d0ee2e211c47f3fd5234d9cf0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
