|
|
DescriptionRemove Page dependency from NetworkStateNotifier
OnLine event notification should be done via observer model so that we can:
- eliminate Page dependency from NetworkStateNotifier
- make it easy to support online event in Workers
BUG=691566
Review-Url: https://codereview.chromium.org/2704083002
Cr-Commit-Position: refs/heads/master@{#452511}
Committed: https://chromium.googlesource.com/chromium/src/+/fbb1bb97ae95653a5d4cc6b55d3f71260ac5fc5d
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : DOMWindow -> Document #
Total comments: 10
Patch Set 4 : . #Patch Set 5 : addressed comments #
Total comments: 6
Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : rebase etc #Patch Set 9 : nullcheck for tests #Messages
Total messages: 49 (34 generated)
The CQ bit was checked by kinuko@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.
Description was changed from ========== Remove Page dependency from NetworkStateNotifier OnLine event notification should be done via observer model so that we can: - eliminate Page dependency from NetworkStateNotifier - make it easy to support online event in Workers BUG=691566 ========== to ========== Remove Page dependency from NetworkStateNotifier OnLine event notification should be done via observer model so that we can: - eliminate Page dependency from NetworkStateNotifier - make it easy to support online event in Workers BUG=691566 ==========
kinuko@chromium.org changed reviewers: + dcheng@chromium.org, jkarlin@chromium.org
This change turns out to be a little awkward than I had expected. Could you review? jkarlin@: NetworkStateNotifier changes and overall dcheng@: Page and LocalDOMWindow (and whatever you care about)
https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:378: m_networkStateObserver = new NetworkStateObserver(*this); The main question I have about this CL is why NetworkStateObserver lives on LocalDOMWindow if it is a ContextLifecycleObserver. Should it be a member of Document instead?
On 2017/02/22 08:05:13, dcheng wrote: > https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): > > https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:378: > m_networkStateObserver = new NetworkStateObserver(*this); > The main question I have about this CL is why NetworkStateObserver lives on > LocalDOMWindow if it is a ContextLifecycleObserver. Should it be a member of > Document instead? Yeah I would have had the same question... I initially chose it as it dispatches event on Window, but probably it's better to be on Document. I'll update the CL.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
The CQ bit was unchecked by kinuko@chromium.org
The CQ bit was checked by kinuko@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...
PTAL https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:378: m_networkStateObserver = new NetworkStateObserver(*this); On 2017/02/22 08:05:13, dcheng wrote: > The main question I have about this CL is why NetworkStateObserver lives on > LocalDOMWindow if it is a ContextLifecycleObserver. Should it be a member of > Document instead? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kinuko@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...
https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:415: NetworkStateObserver(Document& document) Nit: explicit https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:418: this, TaskRunnerHelper::get(TaskType::Networking, m_document).get()); Is it safe to pass (and store) the WebTaskRunner as a raw pointer? https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:120: void removeConnectionObserver(NetworkStateObserver*, WebTaskRunner*); I find it surprising the unregister method takes a WebTaskRunner. IMO it should just be based on the observer pointer. Can we add a TODO to address this in a followup? https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:128: Vector<size_t> zeroedObservers; // Indices in observers that are 0. Unrelated: hmm... we have a similar ObserverList in base with a thread-safe version. I wonder if we should be sharing code eventually... (This is fine though, no need to try to change it in this CL)
The CQ bit was checked by kinuko@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! Updated. https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:415: NetworkStateObserver(Document& document) On 2017/02/22 09:23:15, dcheng wrote: > Nit: explicit Done. https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:418: this, TaskRunnerHelper::get(TaskType::Networking, m_document).get()); On 2017/02/22 09:23:15, dcheng wrote: > Is it safe to pass (and store) the WebTaskRunner as a raw pointer? Hmm, there was an assumption that the task runner must be alive while the associated execution context's alive, but it might not be really guaranteed. Fixed. https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:120: void removeConnectionObserver(NetworkStateObserver*, WebTaskRunner*); On 2017/02/22 09:23:15, dcheng wrote: > I find it surprising the unregister method takes a WebTaskRunner. IMO it should > just be based on the observer pointer. Can we add a TODO to address this in a > followup? This class groups observers by WebTaskRunner's to reduce the # of cross-thread map access, and that's the reason we need WebTaskRunner in both addition and removal. (That's said I agree that this API looks a bit unusual) https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:128: Vector<size_t> zeroedObservers; // Indices in observers that are 0. On 2017/02/22 09:23:15, dcheng wrote: > Unrelated: hmm... we have a similar ObserverList in base with a thread-safe > version. I wonder if we should be sharing code eventually... > > (This is fine though, no need to try to change it in this CL) I plan to move this class under platform/ after this CL lands, I'll try to see if we can use base::ObserverList then.
lg, just a few comments/questions https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:175: NetworkStateObserver(LocalDOMWindow& window) I don't have much blink experience. Should this be a pointer or a reference? It doesn't seem like an out-param to me so perhaps a pointer would be better? https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:190: networkStateNotifier().removeOnLineObserver( I'm not familiar with this code. Are we guaranteed that contextDestroyed will be called before destruction of this object? If not, should we add a destructor which calls removeOnLineObserver? https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:128: Vector<size_t> zeroedObservers; // Indices in observers that are 0. On 2017/02/22 13:17:27, kinuko wrote: > On 2017/02/22 09:23:15, dcheng wrote: > > Unrelated: hmm... we have a similar ObserverList in base with a thread-safe > > version. I wonder if we should be sharing code eventually... > > > > (This is fine though, no need to try to change it in this CL) > > I plan to move this class under platform/ after this CL lands, I'll try to see > if we can use base::ObserverList then. This was written before we had base in blink. I'd be happy to see it go and use the base class instead. https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:167: void notifyObserversOnTaskRunner(ObserverListMap*, Why is ObserverListMap and pointer here but passed by reference in notifyObservers()?
Thanks! Inline responses only. https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:175: NetworkStateObserver(LocalDOMWindow& window) On 2017/02/22 18:38:08, jkarlin wrote: > I don't have much blink experience. Should this be a pointer or a reference? It > doesn't seem like an out-param to me so perhaps a pointer would be better? Yeah I know how you feel, in my understanding we prefer reference when we're sure the param is non-null in Blink. (I can't find it quickly but we had a discussion thread on blink-dev sometime ago) (Btw I moved this class into Document in the newer PS, though the same discussion applies) https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:190: networkStateNotifier().removeOnLineObserver( On 2017/02/22 18:38:08, jkarlin wrote: > I'm not familiar with this code. Are we guaranteed that contextDestroyed will be > called before destruction of this object? If not, should we add a destructor > which calls removeOnLineObserver? Yes, it's guaranteed. (This is GC'ed class, adding cleanup in dtor is not really safe) https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:128: Vector<size_t> zeroedObservers; // Indices in observers that are 0. On 2017/02/22 18:38:08, jkarlin wrote: > On 2017/02/22 13:17:27, kinuko wrote: > > On 2017/02/22 09:23:15, dcheng wrote: > > > Unrelated: hmm... we have a similar ObserverList in base with a thread-safe > > > version. I wonder if we should be sharing code eventually... > > > > > > (This is fine though, no need to try to change it in this CL) > > > > I plan to move this class under platform/ after this CL lands, I'll try to see > > if we can use base::ObserverList then. > > This was written before we had base in blink. I'd be happy to see it go and use > the base class instead. Yeah, and we still can't depend on most things in base from core, so let's move this first. https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.h:167: void notifyObserversOnTaskRunner(ObserverListMap*, On 2017/02/22 18:38:08, jkarlin wrote: > Why is ObserverListMap and pointer here but passed by reference in > notifyObservers()? The former takes reference as it's known that it's never nullptr, the latter takes pointer because I needed to work around how crossThreadBind works (I don't like this inconsistency either, a bit sad)
LGTM with nits https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:416: : ContextLifecycleObserver(&document), m_document(&document) { Nit: instead of storing a separate m_document member, let's add a document() helper which does toDocument(getExecutionContext()). https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:208: result.storedValue->value = WTF::wrapUnique(new ObserverList); Nit: WTF::MakeUnique<ObserverList>()
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:416: : ContextLifecycleObserver(&document), m_document(&document) { On 2017/02/23 02:48:30, dcheng wrote: > Nit: instead of storing a separate m_document member, let's add a document() > helper which does toDocument(getExecutionContext()). Done. https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:208: result.storedValue->value = WTF::wrapUnique(new ObserverList); On 2017/02/23 02:48:30, dcheng wrote: > Nit: WTF::MakeUnique<ObserverList>() Done.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kinuko@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm! Thanks for doing this.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2704083002/#ps160001 (title: "rebase etc")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2704083002/#ps180001 (title: "nullcheck for tests")
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": 180001, "attempt_start_ts": 1487860930417230, "parent_rev": "d0b0e164df66904125ecdcf3473212a1d97a11ce", "commit_rev": "fbb1bb97ae95653a5d4cc6b55d3f71260ac5fc5d"}
Message was sent while issue was closed.
Description was changed from ========== Remove Page dependency from NetworkStateNotifier OnLine event notification should be done via observer model so that we can: - eliminate Page dependency from NetworkStateNotifier - make it easy to support online event in Workers BUG=691566 ========== to ========== Remove Page dependency from NetworkStateNotifier OnLine event notification should be done via observer model so that we can: - eliminate Page dependency from NetworkStateNotifier - make it easy to support online event in Workers BUG=691566 Review-Url: https://codereview.chromium.org/2704083002 Cr-Commit-Position: refs/heads/master@{#452511} Committed: https://chromium.googlesource.com/chromium/src/+/fbb1bb97ae95653a5d4cc6b55d3f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/fbb1bb97ae95653a5d4cc6b55d3f... |