Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739573005:
Rename Document::widgetForElement to frameViewBaseForElement (Closed)
|
Created:
3 years, 9 months ago by joelhockey Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRename Document::widgetForElement to frameViewBaseForElement
BUG=697351
Review-Url: https://codereview.chromium.org/2739573005
Cr-Commit-Position: refs/heads/master@{#456949}
Committed: https://chromium.googlesource.com/chromium/src/+/e0604382f6f7580f4421387f35ef2fe559ab107b
Patch Set 1 #Patch Set 2 : Move Document::widgetForElement to Node::frameViewBase #Patch Set 3 : Move Document::widgetForElement to Node::frameViewBase #
Total comments: 4
Patch Set 4 : Move frameViewBase back to Document #Patch Set 5 : Move frameViewBase back to Document #Messages
Total messages: 48 (25 generated)
The CQ bit was checked by joelhockey@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...
joelhockey@chromium.org changed reviewers: + sashab@chromium.org
joelhockey@chromium.org changed reviewers: + slangley@chromium.org
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 joelhockey@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by joelhockey@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Is this linked to the right bug? If so, please add some more context to the CL description, I'm not sure what this patch is for :)
Description was changed from ========== Move Document::widgetForElement to Node::frameViewBase BUG=697351 ========== to ========== Move Document::widgetForElement to Node::frameViewBase BUG=697351 ==========
joelhockey@chromium.org changed reviewers: + haraken@chromium.org
On 2017/03/08 23:00:50, sashab wrote: > Is this linked to the right bug? If so, please add some more context to the CL > description, I'm not sure what this patch is for :) Yes, this is linked to the right bug for renaming Widget to FrameViewBase. In this case, the method is renamed from widgetForElement to frameViewBase. The method is also being moved from Document to Node where it makes more sense given that it is accessing Node::layoutObject. Haraken suggested that I move this method from Document to Element in https://codereview.chromium.org/2727913005/, but on closer inspection, Node is the most approprate.
Ahh, I see. Yeah, when I saw the bug was for a rename, I was confused why this was a non-rename change :) Maybe the CL description could be like: Rename widgetForElement to frameViewBase. Rename widgetForElement to frameViewBase.... Also move the method from Document to Node, which makes more sense because... That would make it clearer that the method moving is a side effect of the rename, but the patch is really just renaming the method :) You could also split it across two patches, but seeing as its already done that's up to you.
On 2017/03/10 00:58:30, sashab wrote: > Ahh, I see. Yeah, when I saw the bug was for a rename, I was confused why this > was a non-rename change :) > > Maybe the CL description could be like: > > Rename widgetForElement to frameViewBase. > > Rename widgetForElement to frameViewBase.... > Also move the method from Document to Node, which makes more sense because... > > That would make it clearer that the method moving is a side effect of the > rename, but the patch is really just renaming the method :) > > You could also split it across two patches, but seeing as its already done > that's up to you. sashab / haraken, is anyone happy to give LGTM?
Description was changed from ========== Move Document::widgetForElement to Node::frameViewBase BUG=697351 ========== to ========== Move Document::widgetForElement to Node::frameViewBase Rename widgetForElement to frameViewBase. Also move the method from Document to Node which makes more sense since this method is not related to Document, but is accessing Node::layoutObject. BUG=697351 ==========
> sashab / haraken, is anyone happy to give LGTM? Description now updated
https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:600: return layoutObject && layoutObject->isLayoutPart() I actually don't know if this is easier to read :( The early exit was simple and matches the rest of the codebase... Can we leave it that way? :)
https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:600: return layoutObject && layoutObject->isLayoutPart() On 2017/03/10 04:30:13, sashab wrote: > I actually don't know if this is easier to read :( The early exit was simple and > matches the rest of the codebase... Can we leave it that way? :) I wrote it to match the surrounding, related code. See the 2 methods above it.
LGTM but please dont land without an LGTM from haraken :) https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:600: return layoutObject && layoutObject->isLayoutPart() On 2017/03/10 04:37:53, joelhockey wrote: > On 2017/03/10 04:30:13, sashab wrote: > > I actually don't know if this is easier to read :( The early exit was simple > and > > matches the rest of the codebase... Can we leave it that way? :) > > I wrote it to match the surrounding, related code. See the 2 methods above it. Oh, so you did, sorry about that :)
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
This is only used in Document.cpp, we should avoid making the API surface of Node bigger like this (it already has many dozens of methods). It makes more sense as a function in the location that uses it.
https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2739573005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:39: #include "platform/FrameViewBase.h" Node.h is included in nearly every file, so this is transitively including FrameViewBase in nearly every file. You'd want to forward declare this class instead if we added this method.
(I'd defer this review to Elliott :-)
On 2017/03/10 09:53:23, haraken wrote: > (I'd defer this review to Elliott :-) I'm going to move this back to a static function as Document::frameViewBase(Element). It is likely that many methods like this will be removed completely or renamed to be specific about FrameView, Scrollbar or Plugin once I remove the widget tree and remove the FrameViewBase base class.
Description was changed from ========== Move Document::widgetForElement to Node::frameViewBase Rename widgetForElement to frameViewBase. Also move the method from Document to Node which makes more sense since this method is not related to Document, but is accessing Node::layoutObject. BUG=697351 ========== to ========== Rename Document::widgetForElement to frameViewBase BUG=697351 ==========
On 2017/03/13 00:10:05, joelhockey wrote: > On 2017/03/10 09:53:23, haraken wrote: > > (I'd defer this review to Elliott :-) > > I'm going to move this back to a static function as > Document::frameViewBase(Element). It is likely that many methods like this will > be removed completely or renamed to be specific about FrameView, Scrollbar or > Plugin once I remove the widget tree and remove the FrameViewBase base class. Elliott PTAL
Can we keep the old name like ForSomething? Btw I think this FrameViewBase name is really confusing, since Scrollbars and Plugins are "FrameViewBase" now which feels like clear than Widget which described a class of things, like FrameViewBase doesn't tell you much. I'd focus on removing the base class entirely instead of renaming bits. lgtm
On 2017/03/13 00:40:43, esprehn wrote: > Can we keep the old name like ForSomething? Btw I think this FrameViewBase name > is really confusing, since Scrollbars and Plugins are "FrameViewBase" now which > feels like clear than Widget which described a class of things, like > FrameViewBase doesn't tell you much. I'd focus on removing the base class > entirely instead of renaming bits. > > lgtm I will close this change without submitting. Once widget tree is removed, I can reconsider the best change here.
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + skobes@chromium.org, wkorman@chromium.org
Message was sent while issue was closed.
+skobes +wkorman What's your plan for removing the widget tree?
Message was sent while issue was closed.
On 2017/03/13 10:59:24, haraken wrote: > +skobes +wkorman > > What's your plan for removing the widget tree? We plan to work on it, it is currently paused, and I expect to start on it again in Q2. I've made two initial exploratory stabs at it so far and better understand the work, but there is more to do. There are two tracking bugs with some notes, but part of restarting work will be to frame the plan in more detail. The bugs are http://crbug.com/637460 and http://crbug.com/634143.
Message was sent while issue was closed.
On 2017/03/13 23:31:13, wkorman wrote: > On 2017/03/13 10:59:24, haraken wrote: > > +skobes +wkorman > > > > What's your plan for removing the widget tree? > > We plan to work on it, it is currently paused, and I expect to start on it again > in Q2. I've made two initial exploratory stabs at it so far and better > understand the work, but there is more to do. > > There are two tracking bugs with some notes, but part of restarting work will be > to frame the plan in more detail. The bugs are http://crbug.com/637460 and > http://crbug.com/634143. Sounds awesome! Joel: How much "widget"s are left in the code base? If it's few, I guess it's better to finish the rename to avoid confusion in the code base (rather than leaving both "widget"s and "frameViewBase"s).
Message was sent while issue was closed.
On 2017/03/14 at 08:06:35, haraken wrote: > On 2017/03/13 23:31:13, wkorman wrote: > > On 2017/03/13 10:59:24, haraken wrote: > > > +skobes +wkorman > > > > > > What's your plan for removing the widget tree? > > > > We plan to work on it, it is currently paused, and I expect to start on it again > > in Q2. I've made two initial exploratory stabs at it so far and better > > understand the work, but there is more to do. > > > > There are two tracking bugs with some notes, but part of restarting work will be > > to frame the plan in more detail. The bugs are http://crbug.com/637460 and > > http://crbug.com/634143. > > Sounds awesome! > > Joel: How much "widget"s are left in the code base? If it's few, I guess it's better to finish the rename to avoid confusion in the code base (rather than leaving both "widget"s and "frameViewBase"s). There are still a few. Let me see if I can do a simple rename of all of them and send you a CL.
The CQ bit was checked by joelhockey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2739573005/#ps80001 (title: "Move frameViewBase back to Document")
Description was changed from ========== Rename Document::widgetForElement to frameViewBase BUG=697351 ========== to ========== Rename Document::widgetForElement to frameViewBaseForElement BUG=697351 ==========
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": 1489537849177620,
"parent_rev": "335e4d5926d1360df2a0f973a760ae9a9abe054a", "commit_rev":
"e0604382f6f7580f4421387f35ef2fe559ab107b"}
Message was sent while issue was closed.
Description was changed from ========== Rename Document::widgetForElement to frameViewBaseForElement BUG=697351 ========== to ========== Rename Document::widgetForElement to frameViewBaseForElement BUG=697351 Review-Url: https://codereview.chromium.org/2739573005 Cr-Commit-Position: refs/heads/master@{#456949} Committed: https://chromium.googlesource.com/chromium/src/+/e0604382f6f7580f4421387f35ef... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e0604382f6f7580f4421387f35ef... |
