Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(377)

Issue 2704083002: Remove Page dependency from NetworkStateNotifier (Closed)

Created:
3 years, 10 months ago by kinuko
Modified:
3 years, 10 months ago
Reviewers:
jkarlin, dcheng
CC:
chromium-reviews, blink-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+305 lines, -136 lines) Patch
M third_party/WebKit/Source/core/dom/Document.h View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 4 chunks +36 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/NetworkStateNotifier.h View 1 2 3 4 5 chunks +36 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp View 1 2 3 4 5 6 7 chunks +109 lines, -56 lines 0 comments Download
M third_party/WebKit/Source/core/page/NetworkStateNotifierTest.cpp View 3 4 5 6 7 8 21 chunks +115 lines, -36 lines 0 comments Download
M third_party/WebKit/Source/core/page/Page.h View 1 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/page/Page.cpp View 1 2 1 chunk +0 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -6 lines 0 comments Download

Messages

Total messages: 49 (34 generated)
kinuko
This change turns out to be a little awkward than I had expected. Could you ...
3 years, 10 months ago (2017-02-20 03:28:11 UTC) #7
dcheng
https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp#newcode378 third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:378: m_networkStateObserver = new NetworkStateObserver(*this); The main question I have ...
3 years, 10 months ago (2017-02-22 08:05:13 UTC) #8
kinuko
On 2017/02/22 08:05:13, dcheng wrote: > https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp > File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): > > https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp#newcode378 > ...
3 years, 10 months ago (2017-02-22 08:11:21 UTC) #9
kinuko
PTAL https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp#newcode378 third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:378: m_networkStateObserver = new NetworkStateObserver(*this); On 2017/02/22 08:05:13, dcheng ...
3 years, 10 months ago (2017-02-22 08:38:27 UTC) #14
dcheng
https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode415 third_party/WebKit/Source/core/dom/Document.cpp:415: NetworkStateObserver(Document& document) Nit: explicit https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode418 third_party/WebKit/Source/core/dom/Document.cpp:418: this, TaskRunnerHelper::get(TaskType::Networking, m_document).get()); ...
3 years, 10 months ago (2017-02-22 09:23:17 UTC) #19
kinuko
Thanks! Updated. https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode415 third_party/WebKit/Source/core/dom/Document.cpp:415: NetworkStateObserver(Document& document) On 2017/02/22 09:23:15, dcheng wrote: ...
3 years, 10 months ago (2017-02-22 13:17:27 UTC) #24
jkarlin
lg, just a few comments/questions https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp#newcode175 third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:175: NetworkStateObserver(LocalDOMWindow& window) I don't ...
3 years, 10 months ago (2017-02-22 18:38:09 UTC) #25
kinuko
Thanks! Inline responses only. https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2704083002/diff/20001/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp#newcode175 third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:175: NetworkStateObserver(LocalDOMWindow& window) On 2017/02/22 18:38:08, ...
3 years, 10 months ago (2017-02-23 01:15:35 UTC) #26
dcheng
LGTM with nits https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Source/core/dom/Document.cpp#newcode416 third_party/WebKit/Source/core/dom/Document.cpp:416: : ContextLifecycleObserver(&document), m_document(&document) { Nit: instead ...
3 years, 10 months ago (2017-02-23 02:48:30 UTC) #27
kinuko
Thanks! https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2704083002/diff/80001/third_party/WebKit/Source/core/dom/Document.cpp#newcode416 third_party/WebKit/Source/core/dom/Document.cpp:416: : ContextLifecycleObserver(&document), m_document(&document) { On 2017/02/23 02:48:30, dcheng ...
3 years, 10 months ago (2017-02-23 04:58:16 UTC) #30
jkarlin
lgtm! Thanks for doing this.
3 years, 10 months ago (2017-02-23 12:48:20 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2704083002/160001
3 years, 10 months ago (2017-02-23 13:02:36 UTC) #41
commit-bot: I haz the power
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_chromeos_rel_ng/builds/370953)
3 years, 10 months ago (2017-02-23 14:29:05 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2704083002/180001
3 years, 10 months ago (2017-02-23 14:42:30 UTC) #46
commit-bot: I haz the power
3 years, 10 months ago (2017-02-23 16:13:13 UTC) #49
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/fbb1bb97ae95653a5d4cc6b55d3f...

Powered by Google App Engine
This is Rietveld 408576698