Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783543004:
Move registration of LocalFrame mojo interfaces to ModulesInitializer. (Closed)
DescriptionMove registration of LocalFrame mojo interfaces to ModulesInitializer.
To enable this, added a method to register initilization callbacks for
LocalFrame initization. This method adds the callbacks to a static vector, and
the callbacks are executed when a new LocalFrame is initizialized.
This is part of the work to remove the dependency between WebLocalFrameImpl and
modules/*.
BUG=703405
Review-Url: https://codereview.chromium.org/2783543004
Cr-Commit-Position: refs/heads/master@{#460684}
Committed: https://chromium.googlesource.com/chromium/src/+/94c0ec2f5e66cf6939523e14cf4db4d3267d1ef1
Patch Set 1 #Patch Set 2 : Remove unused headers. #
Total comments: 14
Patch Set 3 : Address code review feedback. #
Total comments: 8
Patch Set 4 : Address codereview comments. #Patch Set 5 : Move modules initialization to the first operation in LocalFrame::Init as the loader can actually d… #
Messages
Total messages: 24 (10 generated)
slangley@chromium.org changed reviewers: + sashab@chromium.org
https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:258: std::vector<LocalFrame::frame_init_callback>& getInitiailizationVector() { getInitiailizationVector -> getInitializationVector https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:262: return iv; initializationVector https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:287: for (auto& cb : getInitiailizationVector()) { choose better var name than cb. maybe initializationVectorCallback https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:862: void LocalFrame::registerInitializationCallback(frame_init_callback cb) { why does frame_init_callback have underscores? what style is that? https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:227: In CL description: removed -> remove https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:233: // were added using registerInitializationCallback, and there is no checks is -> are https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:91: LocalFrame::registerInitializationCallback([](LocalFrame* frame) { I can't read this code. Is this lambdas? I believe you if you say it works
Description was changed from ========== Move registration of LocalFrame mojo interfaces to ModulesInitializer. To enable this, added a method to register initilization callbacks for LocalFrame initization. This method adds the callbacks to a static vector, and the callbacks are executed when a new LocalFrame is initizialized. This is part of the work to removed the dependency between WebLocalFrameImpl and modules/*. BUG=703405 ========== to ========== Move registration of LocalFrame mojo interfaces to ModulesInitializer. To enable this, added a method to register initilization callbacks for LocalFrame initization. This method adds the callbacks to a static vector, and the callbacks are executed when a new LocalFrame is initizialized. This is part of the work to remove the dependency between WebLocalFrameImpl and modules/*. BUG=703405 ==========
ptal https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:258: std::vector<LocalFrame::frame_init_callback>& getInitiailizationVector() { On 2017/03/29 at 00:33:23, sashab wrote: > getInitiailizationVector -> getInitializationVector Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:262: return iv; On 2017/03/29 at 00:33:23, sashab wrote: > initializationVector Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:287: for (auto& cb : getInitiailizationVector()) { On 2017/03/29 at 00:33:23, sashab wrote: > choose better var name than cb. maybe initializationVectorCallback Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:862: void LocalFrame::registerInitializationCallback(frame_init_callback cb) { On 2017/03/29 at 00:33:23, sashab wrote: > why does frame_init_callback have underscores? what style is that? Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:227: On 2017/03/29 at 00:33:23, sashab wrote: > In CL description: removed -> remove Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:233: // were added using registerInitializationCallback, and there is no checks On 2017/03/29 at 00:33:23, sashab wrote: > is -> are Done https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2783543004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:91: LocalFrame::registerInitializationCallback([](LocalFrame* frame) { On 2017/03/29 at 00:33:23, sashab wrote: > I can't read this code. Is this lambdas? > > I believe you if you say it works lambdas - so hot right now
LGTM
LGTM if lambda methods are OK
slangley@chromium.org changed reviewers: + haraken@chromium.org
haraken@ - ptal. This replaces the previous unsubmitted work on AppBannerController.
https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:260: std::vector<LocalFrame::FrameInitCallback>, initializationVector, Use WTF::Vector. Blink doesn't use std::vector. Also LocalFrame is not accessible from non-main threads. You can use DEFINE_STATIC_LOCAL. https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:863: getInitializationVector().push_back(cb); cb => callback Blink prefers a fully qualified name. https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:232: // LocalFrame is initilized. Callbacks are executed in the order that they initialized https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:93: &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame))); You can combine these two callbacks into one. Also I don't think the callback needs to be a vector. LocalFrame::m_frameInitializationCallbackInModule would be sufficient. Then you can add DCHECK(m_frameInitializationCallbackInModule) in LocalFrame::init().
haraken@ - ptal. Note responses to your original comments. Thanks! https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:260: std::vector<LocalFrame::FrameInitCallback>, initializationVector, On 2017/03/29 at 08:10:24, haraken wrote: > Use WTF::Vector. Blink doesn't use std::vector. > > Also LocalFrame is not accessible from non-main threads. You can use DEFINE_STATIC_LOCAL. Done https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:863: getInitializationVector().push_back(cb); On 2017/03/29 at 08:10:24, haraken wrote: > cb => callback > > Blink prefers a fully qualified name. Done https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.h:232: // LocalFrame is initilized. Callbacks are executed in the order that they On 2017/03/29 at 08:10:24, haraken wrote: > initialized Done https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:93: &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame))); On 2017/03/29 at 08:10:24, haraken wrote: > You can combine these two callbacks into one. > > Also I don't think the callback needs to be a vector. LocalFrame::m_frameInitializationCallbackInModule would be sufficient. Then you can add DCHECK(m_frameInitializationCallbackInModule) in LocalFrame::init(). Partially done. I combined the two into one lambda and added a DCHECK that the vector is not empty, but i left the option to register multiple initialization callbacks as it seemed better to leave this as generic as possible. WDYT?
LGTM > https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/ModulesInitializer.cpp:93: > &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame))); > On 2017/03/29 at 08:10:24, haraken wrote: > > You can combine these two callbacks into one. > > > > Also I don't think the callback needs to be a vector. > LocalFrame::m_frameInitializationCallbackInModule would be sufficient. Then you > can add DCHECK(m_frameInitializationCallbackInModule) in LocalFrame::init(). > > Partially done. > > I combined the two into one lambda and added a DCHECK that the vector is not > empty, but i left the option to register multiple initialization callbacks as it > seemed better to leave this as generic as possible. WDYT? I don't have any strong opinion but modules/ would be the only place that will want to inject the call (we shouldn't increase # of linking units any more).
On 2017/03/30 at 00:17:48, haraken wrote: > LGTM > > > https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > > > https://codereview.chromium.org/2783543004/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/ModulesInitializer.cpp:93: > > &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame))); > > On 2017/03/29 at 08:10:24, haraken wrote: > > > You can combine these two callbacks into one. > > > > > > Also I don't think the callback needs to be a vector. > > LocalFrame::m_frameInitializationCallbackInModule would be sufficient. Then you > > can add DCHECK(m_frameInitializationCallbackInModule) in LocalFrame::init(). > > > > Partially done. > > > > I combined the two into one lambda and added a DCHECK that the vector is not > > empty, but i left the option to register multiple initialization callbacks as it > > seemed better to leave this as generic as possible. WDYT? > > I don't have any strong opinion but modules/ would be the only place that will want to inject the call (we shouldn't increase # of linking units any more). If I can't make use of it outside of modules/ in the near future then I will revert back to a more specific method not backed by vector.
The CQ bit was checked by slangley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2783543004/#ps60001 (title: "Address codereview comments.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by slangley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2783543004/#ps80001 (title: "Move modules initialization to the first operation in LocalFrame::Init as the loader can actually d…")
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": 1490852286482070,
"parent_rev": "65adef848f642a861ff781e24f8aaee11607698e", "commit_rev":
"94c0ec2f5e66cf6939523e14cf4db4d3267d1ef1"}
Message was sent while issue was closed.
Description was changed from ========== Move registration of LocalFrame mojo interfaces to ModulesInitializer. To enable this, added a method to register initilization callbacks for LocalFrame initization. This method adds the callbacks to a static vector, and the callbacks are executed when a new LocalFrame is initizialized. This is part of the work to remove the dependency between WebLocalFrameImpl and modules/*. BUG=703405 ========== to ========== Move registration of LocalFrame mojo interfaces to ModulesInitializer. To enable this, added a method to register initilization callbacks for LocalFrame initization. This method adds the callbacks to a static vector, and the callbacks are executed when a new LocalFrame is initizialized. This is part of the work to remove the dependency between WebLocalFrameImpl and modules/*. BUG=703405 Review-Url: https://codereview.chromium.org/2783543004 Cr-Commit-Position: refs/heads/master@{#460684} Committed: https://chromium.googlesource.com/chromium/src/+/94c0ec2f5e66cf6939523e14cf4d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/94c0ec2f5e66cf6939523e14cf4d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
