Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043863003:
Speculatively launch Service Workers on mouse/touch events. [1/5] (Closed)
|
Created:
4 years, 6 months ago by horo Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, serviceworker-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpeculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
Committed: https://crrev.com/cca7ec0e84c6ea291d90a79bfdcbcd5f0a55d612
Cr-Commit-Position: refs/heads/master@{#401225}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Remove DECLARE_VIRTUAL_TRACE_WRAPPERS #
Total comments: 4
Patch Set 3 : remove TRACE_WRAPPERS #
Total comments: 7
Patch Set 4 : incorporated falken's comment #
Total comments: 8
Patch Set 5 : incorporated falken's comment #
Total comments: 2
Patch Set 6 : incorporated kinuko's comment #Patch Set 7 : NavigationHint #Patch Set 8 : remove RuntimeEnabledFeatures.h include #Patch Set 9 : rebase #
Total comments: 4
Patch Set 10 : incorporated kinuko's comment #
Total comments: 2
Patch Set 11 : move WebNavigationHintType to in NavigationHintSender #
Total comments: 2
Patch Set 12 : use DECLARE_TRACE #
Messages
Total messages: 47 (14 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== 1 BUG= ========== to ========== Speculatively launch Service Workers on mouse/touch events. [1/2] [1/2]: This CL. [2/2]: https://codereview.chromium.org/2043083002/ Demo movie: https://drive.google.com/file/d/0B6skYAFVnosEMWdzRm4yMEtIMkk/view?usp=sharing This CL introduces a new feature named "SpeculativeLaunchServiceWorker". When the feature is enabled, HTMLAnchorElement will have NavigationHintSender. It is similar to PrefetchEventHandler which was removed by https://codereview.chromium.org/344133002. When HTMLAnchorElement receives mousedown events for the left button or gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message will be sent to the browser process. NavigationHintSender::maybeSendNavigationHint() -> sendNavigationHint() -> WebPrescientNetworking::sendNavigationHint() -> PrescientNetworkingDispatcher::sendNavigationHint() -> RendererPreconnect::NavigationHint() -> IPC (NetworkHintsMsg_NavigationHint) In the browser process, ChromeRenderMessageFilter receives the IPC message. The browser side logic will be implemented in another CL: https://codereview.chromium.org/2043083002/ BUG=616502 ==========
Patchset #1 (id:20001) has been deleted
horo@chromium.org changed reviewers: + falken@chromium.org, kouhei@chromium.org
kouhei@, falken@ Could you please review this CL?
kouhei@chromium.org changed reviewers: + haraken@chromium.org
Please split the CLs more: Introduce NavigationHintSender, Use NavigationHintSender. Should we add UMAs to track precision/recall? https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:56: DECLARE_VIRTUAL_TRACE_WRAPPERS(); I'm not sure if we need traceWrappers here. haraken@: Would you review the change here? https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:490: HTMLAnchorElement::NavigationHintSender* HTMLAnchorElement::navigationHintSender() ensureNavigationHintSender()
https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:56: DECLARE_VIRTUAL_TRACE_WRAPPERS(); On 2016/06/09 02:31:13, kouhei wrote: > I'm not sure if we need traceWrappers here. haraken@: Would you review the > change here? You don't need to add TRACE_WRAPPERS. It is needed only when you need to annotate some special reachability between V8 wrappers.
Description was changed from ========== Speculatively launch Service Workers on mouse/touch events. [1/2] [1/2]: This CL. [2/2]: https://codereview.chromium.org/2043083002/ Demo movie: https://drive.google.com/file/d/0B6skYAFVnosEMWdzRm4yMEtIMkk/view?usp=sharing This CL introduces a new feature named "SpeculativeLaunchServiceWorker". When the feature is enabled, HTMLAnchorElement will have NavigationHintSender. It is similar to PrefetchEventHandler which was removed by https://codereview.chromium.org/344133002. When HTMLAnchorElement receives mousedown events for the left button or gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message will be sent to the browser process. NavigationHintSender::maybeSendNavigationHint() -> sendNavigationHint() -> WebPrescientNetworking::sendNavigationHint() -> PrescientNetworkingDispatcher::sendNavigationHint() -> RendererPreconnect::NavigationHint() -> IPC (NetworkHintsMsg_NavigationHint) In the browser process, ChromeRenderMessageFilter receives the IPC message. The browser side logic will be implemented in another CL: https://codereview.chromium.org/2043083002/ BUG=616502 ========== to ========== Speculatively launch Service Workers on mouse/touch events. [1/5] 1/5: Introduce NavigationHintSender. This CL. 2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter https://codereview.chromium.org/2043083002/ 3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter https://codereview.chromium.org/2052613003/ 4/5: Measure the precision of the speculative launch of Service Workers for NavigationHints https://codereview.chromium.org/2045153003/ 5/5: Add flags to enable SupeculativeLaunchServiceWorker https://codereview.chromium.org/2053573002/ This CL introduces NavigationHintSender in HTMLAnchorElement which will send NavigationHints to the browser process. IPC related code will be added by [2/5] https://codereview.chromium.org/2043083002/ BUG=616502 ==========
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
This CL introduces NavigationHintSender in HTMLAnchorElement which will send
NavigationHints to the browser process.
IPC related code will be added by [2/5]
https://codereview.chromium.org/2043083002/
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
This CL introduces NavigationHintSender in HTMLAnchorElement.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
This CL introduces NavigationHintSender in HTMLAnchorElement.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
> Please split the CLs more: Introduce NavigationHintSender, Use > NavigationHintSender. Done. > Should we add UMAs to track precision/recall? We will add "ServiceWorker.NavigationHintPrecision" UMA. https://codereview.chromium.org/2045153003/ And we can measure the efficacy of the feature using ServiceWorker.WorkerPreparationMode of ServiceWorker.ActivatedWorkerPreparationForMainFrame.Time. https://codereview.chromium.org/2039743003/ https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:56: DECLARE_VIRTUAL_TRACE_WRAPPERS(); On 2016/06/09 04:07:29, haraken wrote: > On 2016/06/09 02:31:13, kouhei wrote: > > I'm not sure if we need traceWrappers here. haraken@: Would you review the > > change here? > > You don't need to add TRACE_WRAPPERS. It is needed only when you need to > annotate some special reachability between V8 wrappers. > Removed. Thank you. https://codereview.chromium.org/2043863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:490: HTMLAnchorElement::NavigationHintSender* HTMLAnchorElement::navigationHintSender() On 2016/06/09 02:31:13, kouhei wrote: > ensureNavigationHintSender() Done.
https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:139: DEFINE_TRACE_WRAPPERS(HTMLAnchorElement) ditto https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:91: DECLARE_VIRTUAL_TRACE_WRAPPERS(); Looks still present
https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:139: DEFINE_TRACE_WRAPPERS(HTMLAnchorElement) On 2016/06/09 08:01:19, kouhei wrote: > ditto Done. https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/2043863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:91: DECLARE_VIRTUAL_TRACE_WRAPPERS(); On 2016/06/09 08:01:19, kouhei wrote: > Looks still present Oops! Done.
ps3 lgtm
horo@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@ Could you please review third_party/WebKit/public/platform/WebNavigationHintType.h?
https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:88: if (!url.protocolIsInHTTPFamily()) Why is it restricted to HTTP? Is this because the NavigationHint is for service worker? If so, maybe this function should be named shouldSendNavigationHintForServiceWorker and we could add a comment about how SW is restricted to http? Also technically we should use SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers instead. (But that's a bit annoying because it uses a mutex) https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:95: if (url.hasFragmentIdentifier() && equalIgnoringFragmentIdentifier(document.url(), url)) Is this avoiding the hint if the link is to current document? Why is that needed? Does hasFragmentIdentifier need to be called? https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:101: // Service Worker not to keep the current process running. Ah yes this code is pretty SW-specific but the naming doesn't give a hint of that. https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebNavigationHintType.h:11: Unkown, Unknown
(Looking reasonable to me in general mod the points Matt commented, I'm also concerned a bit how much this change will be SW-specific or not. will take another look after they're addressed)
https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:88: if (!url.protocolIsInHTTPFamily()) On 2016/06/09 13:24:16, falken ooo till Jun 15 wrote: > Why is it restricted to HTTP? Is this because the NavigationHint is for service > worker? If so, maybe this function should be named > shouldSendNavigationHintForServiceWorker and we could add a comment about how SW > is restricted to http? > > Also technically we should use > SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers instead. (But > that's a bit annoying because it uses a mutex) Done. https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:95: if (url.hasFragmentIdentifier() && equalIgnoringFragmentIdentifier(document.url(), url)) On 2016/06/09 13:24:16, falken ooo till Jun 15 wrote: > Is this avoiding the hint if the link is to current document? Why is that > needed? > > Does hasFragmentIdentifier need to be called? When the user clicks a link which is to the current document with #hash, the network request is not fetched. So we don't need to start ServiceWorker. So returns false here. If #hash is not set, the network request is fetched. So we should start ServiceWorker. https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043863003/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebNavigationHintType.h:11: Unkown, On 2016/06/09 13:24:16, falken ooo till Jun 15 wrote: > Unknown Done.
I'm probably outside of my depth (not an OWNER of this code), but some more comments. https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:95: return false; I'm probably just clueless about DOM stuff but why do we need to check frame() and security origin here? If there is a link to a some URL, I'd assume that a navigation to that isn't restricted by this document's origin, since we'd get a new document. However if I were to come across this code I'd be afraid to change it in and cause a security issue. I'd prefer explanatory comments here for better maintainability, unless it's obvious to Blink people. https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:97: return false; Thanks for the explanation of this one, that makes sense to me now. https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:101: // user click the anchor element. In such case we shouldn't start the s/click/clicks https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:102: // Service Worker not to keep the current process running. The first sentence makes sense to me, but the second sentence does not... both the grammar and the meaning. Is it saying you don't want to create the SW in this current process, because the navigation will go to a new process? I thought our SW implementation is supposed to handle the case of SW in one process and page in another. https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:123: Member<NavigationHintSender> m_navigationHintsender; s/sender/Sender
https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:95: return false; On 2016/06/15 13:48:19, falken ooo till Jun 16 wrote: > I'm probably just clueless about DOM stuff but why do we need to check frame() > and security origin here? If there is a link to a some URL, I'd assume that a > navigation to that isn't restricted by this document's origin, since we'd get a > new document. However if I were to come across this code I'd be afraid to change > it in and cause a security issue. > > I'd prefer explanatory comments here for better maintainability, unless it's > obvious to Blink people. Added comment about checking frame(). Ah we don't need to check canDisplay(). Removed. (It was originally from kouhei@'s patch. https://chromiumcodereview.appspot.com/15352003) https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:102: // Service Worker not to keep the current process running. On 2016/06/15 13:48:19, falken ooo till Jun 16 wrote: > The first sentence makes sense to me, but the second sentence does not... both > the grammar and the meaning. Is it saying you don't want to create the SW in > this current process, because the navigation will go to a new process? I > thought our SW implementation is supposed to handle the case of SW in one > process and page in another. I thought that we should not run the SW in the current process if the new navigation creates another new process. But there is no apparent reason. Removed. https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/2043863003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:123: Member<NavigationHintSender> m_navigationHintsender; On 2016/06/15 13:48:19, falken ooo till Jun 16 wrote: > s/sender/Sender Done.
https://codereview.chromium.org/2043863003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:48: class HTMLAnchorElement::NavigationHintSender : public GarbageCollected<HTMLAnchorElement::NavigationHintSender> { Now it looks this class seems to exist primarily for SW, is it intended layering? NavigationHintFoo class names / enum names appear to be very generic on the contrary.
https://codereview.chromium.org/2043863003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:48: class HTMLAnchorElement::NavigationHintSender : public GarbageCollected<HTMLAnchorElement::NavigationHintSender> { On 2016/06/16 06:14:31, kinuko wrote: > Now it looks this class seems to exist primarily for SW, is it intended > layering? NavigationHintFoo class names / enum names appear to be very generic > on the contrary. Added "ForServiceWorkerSender".
I think my question was whether we want to introduce as a SW specific fixture or not. Should HTMLAnchorElement be really aware of it? Should the filtering for SW be done here or in other places? It's not very clear how these decisions are being made.
On 2016/06/16 07:19:12, kinuko wrote: > I think my question was whether we want to introduce as a SW specific fixture or > not. Should HTMLAnchorElement be really aware of it? Should the filtering for > SW be done here or in other places? It's not very clear how these decisions are > being made. I don't have any strong opinion about it. Currently we don't have any plan to use the NavigationHint from anchor elements except for ServiceWorker. So we don't need to have so generic name. But WebNavigationHintForServiceWorkerType.h may be too specific name for adding to public/platform/.
On 2016/06/16 07:40:33, horo (slow till June 20) wrote: > On 2016/06/16 07:19:12, kinuko wrote: > > I think my question was whether we want to introduce as a SW specific fixture > or > > not. Should HTMLAnchorElement be really aware of it? Should the filtering > for > > SW be done here or in other places? It's not very clear how these decisions > are > > being made. > > I don't have any strong opinion about it. > > Currently we don't have any plan to use the NavigationHint from anchor elements > except for ServiceWorker. > So we don't need to have so generic name. > > But WebNavigationHintForServiceWorkerType.h may be too specific name for adding > to public/platform/. Yep... I understand the current code's being added specifically for SW experiment, but it also looks a bit weird to me having these very SW-specific things in html/ and public/platform/. If I'm reading the code right the SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers check seems to be the only SW-specific part for now. Is the check need to be done there?
On 2016/06/16 09:11:39, kinuko wrote: > On 2016/06/16 07:40:33, horo (slow till June 20) wrote: > > On 2016/06/16 07:19:12, kinuko wrote: > > > I think my question was whether we want to introduce as a SW specific > fixture > > or > > > not. Should HTMLAnchorElement be really aware of it? Should the filtering > > for > > > SW be done here or in other places? It's not very clear how these decisions > > are > > > being made. > > > > I don't have any strong opinion about it. > > > > Currently we don't have any plan to use the NavigationHint from anchor > elements > > except for ServiceWorker. > > So we don't need to have so generic name. > > > > But WebNavigationHintForServiceWorkerType.h may be too specific name for > adding > > to public/platform/. > > Yep... I understand the current code's being added specifically for SW > experiment, but it also looks a bit weird to me having these very SW-specific > things in html/ and public/platform/. If I'm reading the code right the > SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers check seems to be > the only SW-specific part for now. Is the check need to be done there? We can check it in the browser process. But if we check here we can reduce unnecessary IPC messages.
On 2016/06/16 09:21:56, horo (slow till June 20) wrote: > On 2016/06/16 09:11:39, kinuko wrote: > > On 2016/06/16 07:40:33, horo (slow till June 20) wrote: > > > On 2016/06/16 07:19:12, kinuko wrote: > > > > I think my question was whether we want to introduce as a SW specific > > fixture > > > or > > > > not. Should HTMLAnchorElement be really aware of it? Should the > filtering > > > for > > > > SW be done here or in other places? It's not very clear how these > decisions > > > are > > > > being made. > > > > > > I don't have any strong opinion about it. > > > > > > Currently we don't have any plan to use the NavigationHint from anchor > > elements > > > except for ServiceWorker. > > > So we don't need to have so generic name. > > > > > > But WebNavigationHintForServiceWorkerType.h may be too specific name for > > adding > > > to public/platform/. > > > > Yep... I understand the current code's being added specifically for SW > > experiment, but it also looks a bit weird to me having these very SW-specific > > things in html/ and public/platform/. If I'm reading the code right the > > SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers check seems to be > > the only SW-specific part for now. Is the check need to be done there? > > We can check it in the browser process. > But if we check here we can reduce unnecessary IPC messages. Is there a chance that we do the filtering in renderer before we send IPC? (Are you going to add mojo ipc or add existing ipc that goes through content?) Also I think filtering this only to http(s) for now probably makes sense (and is easier) with proper commenting (here and at public layer when you're plumbing further e.g. "Sends navigation hints to trigger possible optimizations for navigation. Currently we send this notification only for for http/https family" etc). (falken@- wdyt?)
On 2016/06/17 02:13:02, kinuko wrote: > On 2016/06/16 09:21:56, horo (slow till June 20) wrote: > > On 2016/06/16 09:11:39, kinuko wrote: > > > On 2016/06/16 07:40:33, horo (slow till June 20) wrote: > > > > On 2016/06/16 07:19:12, kinuko wrote: > > > > > I think my question was whether we want to introduce as a SW specific > > > fixture > > > > or > > > > > not. Should HTMLAnchorElement be really aware of it? Should the > > filtering > > > > for > > > > > SW be done here or in other places? It's not very clear how these > > decisions > > > > are > > > > > being made. > > > > > > > > I don't have any strong opinion about it. > > > > > > > > Currently we don't have any plan to use the NavigationHint from anchor > > > elements > > > > except for ServiceWorker. > > > > So we don't need to have so generic name. > > > > > > > > But WebNavigationHintForServiceWorkerType.h may be too specific name for > > > adding > > > > to public/platform/. > > > > > > Yep... I understand the current code's being added specifically for SW > > > experiment, but it also looks a bit weird to me having these very > SW-specific > > > things in html/ and public/platform/. If I'm reading the code right the > > > SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers check seems to > be > > > the only SW-specific part for now. Is the check need to be done there? > > > > We can check it in the browser process. > > But if we check here we can reduce unnecessary IPC messages. > > Is there a chance that we do the filtering in renderer before we send IPC? (Are > you going to add mojo ipc or add existing ipc that goes through content?) > > Also I think filtering this only to http(s) for now probably makes sense (and is > easier) with proper commenting (here and at public layer when you're plumbing > further e.g. "Sends navigation hints to trigger possible optimizations for > navigation. Currently we send this notification only for for http/https family" > etc). (falken@- wdyt?) I'm OK with renaming this something more generic like NavigationHintSender with a comment about "http/https family only". My original problem with the generic name was that the code was SW specific. As you mention now just http/https is the only SW specific part. I wonder if another possible approach is to move NavigationHintSender out of HTMLAnchorElement.cpp and somehow expose a listener/observer API that it can hook into.
Patchset #7 (id:160001) has been deleted
On 2016/06/17 08:37:22, falken wrote: > On 2016/06/17 02:13:02, kinuko wrote: > > On 2016/06/16 09:21:56, horo (slow till June 20) wrote: > > > On 2016/06/16 09:11:39, kinuko wrote: > > > > On 2016/06/16 07:40:33, horo (slow till June 20) wrote: > > > > > On 2016/06/16 07:19:12, kinuko wrote: > > > > > > I think my question was whether we want to introduce as a SW specific > > > > fixture > > > > > or > > > > > > not. Should HTMLAnchorElement be really aware of it? Should the > > > filtering > > > > > for > > > > > > SW be done here or in other places? It's not very clear how these > > > decisions > > > > > are > > > > > > being made. > > > > > > > > > > I don't have any strong opinion about it. > > > > > > > > > > Currently we don't have any plan to use the NavigationHint from anchor > > > > elements > > > > > except for ServiceWorker. > > > > > So we don't need to have so generic name. > > > > > > > > > > But WebNavigationHintForServiceWorkerType.h may be too specific name for > > > > adding > > > > > to public/platform/. > > > > > > > > Yep... I understand the current code's being added specifically for SW > > > > experiment, but it also looks a bit weird to me having these very > > SW-specific > > > > things in html/ and public/platform/. If I'm reading the code right the > > > > SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers check seems > to > > be > > > > the only SW-specific part for now. Is the check need to be done there? > > > > > > We can check it in the browser process. > > > But if we check here we can reduce unnecessary IPC messages. > > > > Is there a chance that we do the filtering in renderer before we send IPC? > (Are > > you going to add mojo ipc or add existing ipc that goes through content?) > > > > Also I think filtering this only to http(s) for now probably makes sense (and > is > > easier) with proper commenting (here and at public layer when you're plumbing > > further e.g. "Sends navigation hints to trigger possible optimizations for > > navigation. Currently we send this notification only for for http/https > family" > > etc). (falken@- wdyt?) > > I'm OK with renaming this something more generic like NavigationHintSender with > a comment about "http/https family only". > > My original problem with the generic name was that the code was SW specific. As > you mention now just http/https is the only SW specific part. > > I wonder if another possible approach is to move NavigationHintSender out of > HTMLAnchorElement.cpp and somehow expose a listener/observer API that it can > hook into. In Patch Set 8, I changed shouldSendNavigationHint() to only check protocolIsInHTTPFamily(). And changed the names to NavigationHint(Sender).
https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:87: if (!url.protocolIsInHTTPFamily()) nit: please add a brief comment about this condition https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebNavigationHintType.h:10: enum class WebNavigationHintType { Sorry to say this after some back and forth, but could we start with non-public enum in this patch and then add this file at public/ when the embedder /content actually start to use it?
https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:87: if (!url.protocolIsInHTTPFamily()) On 2016/06/20 04:53:59, kinuko wrote: > nit: please add a brief comment about this condition Done. https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043863003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebNavigationHintType.h:10: enum class WebNavigationHintType { On 2016/06/20 04:53:59, kinuko wrote: > Sorry to say this after some back and forth, but could we start with non-public > enum in this patch and then add this file at public/ when the embedder /content > actually start to use it? Done.
lgtm https://codereview.chromium.org/2043863003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:46: enum class WebNavigationHintType { nit: It's supposed to be tentative code but it might look saner to have this in the sender class for now
https://codereview.chromium.org/2043863003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:46: enum class WebNavigationHintType { On 2016/06/21 04:51:25, kinuko wrote: > nit: It's supposed to be tentative code but it might look saner to have this in > the sender class for now Done.
not sure if you were waiting for me too... non-owner lgtm
https://codereview.chromium.org/2043863003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:62: DECLARE_VIRTUAL_TRACE(); This should be DECLARE_TRACE, or you can DEFINE_INLINE_TRACE
https://codereview.chromium.org/2043863003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043863003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:62: DECLARE_VIRTUAL_TRACE(); On 2016/06/22 03:38:23, kouhei wrote: > This should be DECLARE_TRACE, or you can DEFINE_INLINE_TRACE Done.
OWNER lgtm
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2043863003/#ps280001 (title: "use DECLARE_TRACE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043863003/280001
Message was sent while issue was closed.
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
Message was sent while issue was closed.
Committed patchset #12 (id:280001)
Message was sent while issue was closed.
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [1/5]
1/5: Introduce NavigationHintSender.
This CL.
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
https://codereview.chromium.org/2043083002/
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
We will introduces a new feature named "SpeculativeLaunchServiceWorker".
When the feature is enabled, HTMLAnchorElement will have NavigationHintSender.
It is similar to PrefetchEventHandler which was removed by
https://codereview.chromium.org/344133002.
When HTMLAnchorElement receives mousedown events for the left button or
gesturetapunconfirmed or gestureshowpress events, a NavigationHint IPC message
will be sent to the browser process.
IPC related code is in [2/5] https://codereview.chromium.org/2043083002/
BUG=616502
Committed: https://crrev.com/cca7ec0e84c6ea291d90a79bfdcbcd5f0a55d612
Cr-Commit-Position: refs/heads/master@{#401225}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/cca7ec0e84c6ea291d90a79bfdcbcd5f0a55d612 Cr-Commit-Position: refs/heads/master@{#401225} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
