Issue 295063002: [NotLanded] Implement the fetching algorithm of the Web Manifest.

Issue 295063002: [NotLanded] Implement the fetching algorithm of the Web Manifest. (Closed)

Created:
6 years, 7 months ago by mlamouri (slow - plz ping)

Modified:
6 years, 3 months ago

Reviewers:
kenneth.christiansen, eseidel, dcheng, abarth-chromium, Mike West, Nate Chapin, Mikhail

CC:
abarth-chromium, blink-reviews, dglazkov+blink, gavinp+loader_chromium.org, jamesr

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Implement the fetching algorithm of the Web Manifest. This is only implementing the fetching part with the callback mechanism it is not implementing the parsing part so the loader will always return an empty WebManifest. BUG=366145

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 14

Patch Set 3 : review comments #

Total comments: 4

Patch Set 4 : #

Total comments: 8

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : rebase #

Total comments: 10

Patch Set 8 : rebase + weblocalframe #

Patch Set 9 : rebase #

Patch Set 10 : #

Patch Set 11 : apply Nate's comments #

Total comments: 10

Patch Set 12 : review comments #

Patch Set 13 : update comments #

Total comments: 14

Patch Set 14 : with webviewhelper #

Patch Set 15 : with webviewhelper #

Total comments: 10

Patch Set 16 : review comments #

Total comments: 12

Patch Set 17 : review comments #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+527 lines, -10 lines)			Patch
M	Source/core/core.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/fetch/FetchInitiatorTypeNames.in	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/fetch/Resource.h	View	1 2 3 4 5 6 7	2 chunks	+4 lines, -2 lines	0 comments	Download
M	Source/core/fetch/Resource.cpp	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/core/fetch/ResourceFetcher.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/fetch/ResourceFetcher.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	8 chunks	+18 lines, -4 lines	0 comments	Download
M	Source/core/loader/FrameLoaderClient.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+6 lines, -2 lines	0 comments	Download
A	Source/core/loader/ManifestLoader.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+42 lines, -0 lines	0 comments	Download
A	Source/core/loader/ManifestLoader.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+90 lines, -0 lines	0 comments	Download
M	Source/web/FrameLoaderClientImpl.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/web/FrameLoaderClientImpl.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+12 lines, -0 lines	0 comments	Download
M	Source/web/WebLocalFrameImpl.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/web/WebLocalFrameImpl.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	3 chunks	+8 lines, -0 lines	0 comments	Download
A	Source/web/tests/ManifestLoaderTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+244 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/cors-anonymous.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+6 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/cors-default.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+6 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/cors-use-credentials.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+6 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/dummy_manifest.json	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+2 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/no-cors.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+6 lines, -0 lines	0 comments	Download
A	Source/web/tests/data/manifest/no-manifest.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/web/web.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+1 line, -0 lines	0 comments	Download
A	public/platform/WebManifest.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+19 lines, -0 lines	0 comments	Download
A	public/platform/WebManifestError.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+17 lines, -0 lines	0 comments	Download
M	public/web/WebFrameClient.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+13 lines, -2 lines	0 comments	Download
M	public/web/WebLocalFrame.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 48 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dcheng

https://codereview.chromium.org/295063002/diff/1/Source/core/loader/ManifestLoaderTest.cpp File Source/core/loader/ManifestLoaderTest.cpp (right): https://codereview.chromium.org/295063002/diff/1/Source/core/loader/ManifestLoaderTest.cpp#newcode168 Source/core/loader/ManifestLoaderTest.cpp:168: ManifestLoader::LoadManifest(request(), document()); Is there any particular reason not to ...

6 years, 7 months ago (2014-05-20 20:11:29 UTC) #2

mlamouri (slow - plz ping)

6 years, 7 months ago (2014-05-20 20:17:03 UTC) #3

mlamouri (slow - plz ping)

dcheng@ and eseidel@, could you both review this CL? Technically eseidel's review would be enough ...

6 years, 7 months ago (2014-05-20 22:31:07 UTC) #4

dcheng

Just some high-level comments. I'll have to think about how to adapt the test helpers, ...

6 years, 7 months ago (2014-05-20 22:40:51 UTC) #5

mlamouri (slow - plz ping)

I applied your review comments. PTAL https://codereview.chromium.org/295063002/diff/20001/Source/core/loader/ManifestLoader.cpp File Source/core/loader/ManifestLoader.cpp (right): https://codereview.chromium.org/295063002/diff/20001/Source/core/loader/ManifestLoader.cpp#newcode22 Source/core/loader/ManifestLoader.cpp:22: ManifestLoader* leakedManifestLoader ALLOW_UNUSED ...

6 years, 7 months ago (2014-05-21 09:51:12 UTC) #6

mlamouri (slow - plz ping)

Friendly reminder: dcheng@ and eseidel@, could you have a look at this CL? Thanks :)

6 years, 7 months ago (2014-05-22 10:46:46 UTC) #7

dcheng

Overall, I'm probably not a good reviewer for this change so I'm just going to ...

6 years, 7 months ago (2014-05-23 01:17:44 UTC) #8

Nate Chapin

I'm a bit confused, why is the loading of a manifest triggered by the embedder ...

6 years, 7 months ago (2014-05-23 17:09:41 UTC) #9

mlamouri (slow - plz ping)

On 2014/05/23 17:09:41, Nate Chapin wrote: > I'm a bit confused, why is the loading ...

6 years, 7 months ago (2014-05-23 18:28:27 UTC) #10

Nate Chapin

On 2014/05/23 18:28:27, Mounir Lamouri wrote: > On 2014/05/23 17:09:41, Nate Chapin wrote: > > ...

6 years, 7 months ago (2014-05-23 18:38:04 UTC) #11

mlamouri (slow - plz ping)

https://codereview.chromium.org/295063002/diff/40001/Source/core/loader/ManifestLoaderTest.cpp File Source/core/loader/ManifestLoaderTest.cpp (right): https://codereview.chromium.org/295063002/diff/40001/Source/core/loader/ManifestLoaderTest.cpp#newcode89 Source/core/loader/ManifestLoaderTest.cpp:89: document()->updateSecurityOrigin(SecurityOrigin::create(url)); On 2014/05/23 01:17:45, dcheng wrote: > I'm not ...

6 years, 6 months ago (2014-05-29 15:14:05 UTC) #13

mlamouri (slow - plz ping)

On 2014/05/23 18:38:04, Nate Chapin wrote: > On 2014/05/23 18:28:27, Mounir Lamouri wrote: > > ...

6 years, 6 months ago (2014-05-29 15:15:18 UTC) #14

Nate Chapin

On 2014/05/29 15:15:18, Mounir Lamouri wrote: > On 2014/05/23 18:38:04, Nate Chapin wrote: > > ...

6 years, 6 months ago (2014-05-29 18:18:14 UTC) #15

mlamouri (slow - plz ping)

On 2014/05/29 18:18:14, Nate Chapin wrote: > On 2014/05/29 15:15:18, Mounir Lamouri wrote: > > ...

6 years, 6 months ago (2014-05-30 13:09:19 UTC) #16

On 2014/05/29 18:18:14, Nate Chapin wrote:
> On 2014/05/29 15:15:18, Mounir Lamouri wrote:
> > On 2014/05/23 18:38:04, Nate Chapin wrote:
> > > On 2014/05/23 18:28:27, Mounir Lamouri wrote:
> > > > On 2014/05/23 17:09:41, Nate Chapin wrote:
> > > > > I'm a bit confused, why is the loading of a manifest triggered by the
> > > embedder
> > > > > calling WebFrame::loadManifest, rather than being decided internally
to
> > > blink?
> > > > 
> > > > We want to lazily load the manifest at a time decided by Chrome. It
might
> > very
> > > > likely be when the user press "Add to homescreen" to start with.
> > > 
> > > Ok. Lazy loading makes sense, but it's not clear to me that blink should
> know
> > > about it. What are the advantages/disadvantages of this method compared to
> > blink
> > > creating a WebManifestRequest as soon as the manifest link is parsed, and
> > > letting the browser process decide when to actually send the request?
> > 
> > Nate, I am not sure what you meant here. Could you rephrase this perhaps?
> 
> Lazy loading make sense. The question is where it is implemented.

I discussed that part with abarth and he told me that the loading will anyway
have to be done in Blink if I want to take into account CORS so I would be
better off having the entire request handled in Blink. I might have
misunderstood something given that I don't know how much Chromium itself could
handle that without relying on Blink.

> Obviously, one option is the way this patch implements it. blink parses the
> <link rel=manifest>, stores that information, and waits for a call to come in
> from a WebFrameClient asking blink to actually start loading the manifest.
> 
> An alternative is for blink to parse the <link rel=manifest> and immediately
> create a WebManifestRequest and send it to the browser process. The browser
> process then holds on to that state and waits for an action (e.g., Add to
> homescreen) before actually sending the request out to the network.

I think that my solution is more flexible in the sense that the embedder doesn't
have to listen to manifest changes. We can imagine UC where we want to know
whether there is a manifest or not before actually doing anything. For example,
the "Add to homescreen" button might be shown higher in the menu if the page has
a manifest. However, we have other features that are considering using the
manifest that would just request the manifest when needed and knowing in advance
if there is one wouldn't be important.

> The option you've chosen feels complicated to me, but maybe the second option
is
> just as bad. I'm interested in whether you've considered alternatives, and if
> so, why you chose this technique.

So, it's not clear to me what is complicated in my solution but I also have a
hard time understanding the alternative. I'm not sure why passing a Request from
Blink to the embedder would be better but I might miss something. One major
issue with your approach I think is that it doesn't take into account that the
DOM is always moving and you could get multiple different manifest during the
life time of a page (it's not something we expect but we should design the code
around the assumption of that being possible).

Nate Chapin

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/ManifestLoader.cpp File Source/core/loader/ManifestLoader.cpp (right): https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/ManifestLoader.cpp#newcode23 Source/core/loader/ManifestLoader.cpp:23: ManifestLoader* manifestLoader = new ManifestLoader(request); Should this be an ...

6 years, 6 months ago (2014-06-03 18:32:41 UTC) #17

Nate Chapin

On 2014/06/03 18:32:41, Nate Chapin wrote: > > https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManifestRequest.h#newcode16 > public/platform/WebManifestRequest.h:16: class WebManifestRequest { > ...

6 years, 6 months ago (2014-06-03 18:34:36 UTC) #18

mlamouri (slow - plz ping)

6 years, 6 months ago (2014-06-03 20:53:10 UTC) #19

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
File Source/core/loader/ManifestLoader.cpp (right):

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
Source/core/loader/ManifestLoader.cpp:23: ManifestLoader* manifestLoader = new
ManifestLoader(request);
On 2014/06/03 18:32:42, Nate Chapin wrote:
> Should this be an OwnPtr<> that gets leaked if we actually start the load?
> PingLoader::start uses a method similar to this.
> 
> startLoading() would need to return a boolean, but we wouldn't need to have a
> "delete this;" in every early-exit case in start loading.

I did something slightly different with OwnPtr<>(this) being instantiated on the
stack of ::startLoading() and ::notifyFinished() so |this| gets deleted when
those functions are left unless .leakPtr() is called.

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
Source/core/loader/ManifestLoader.cpp:51: if (!crossOriginMode.isNull()) {
On 2014/06/03 18:32:42, Nate Chapin wrote:
> Nit: { } unnecessary.

Done.

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
File Source/core/loader/ManifestLoader.h (right):

https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
Source/core/loader/ManifestLoader.h:31: virtual void notifyFinished(Resource*)
OVERRIDE;
On 2014/06/03 18:32:42, Nate Chapin wrote:
> Nit: should this be protected or private instead of public?

Good catch! :)

https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
File public/platform/WebManifestRequest.h (right):

https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
public/platform/WebManifestRequest.h:16: class WebManifestRequest {
On 2014/06/03 18:32:42, Nate Chapin wrote:
> I think part of my confusion about this patch is that this probably shouldn't
be
> in public/platform/, as it's primarily a callback to notify high level code
> about something blink is doing, not a low level API blink uses to do its job.
> 
> The way we usually do this is to have a ManifestLoaderClient next to the
> ManifestLoader in Source/core/loader/. ManifestLoaderClient is then
implemented
> in Source/web/ by a ManifestLoaderClientImpl, which notifies a
WebManifestClient
> in public/web/. :-/
> 
> At least, that's how I understand it. eseidel or one of the other
public/OWNERS
> might have a better idea.

I will ping eseidel. My design sounds simpler but if I'm not doing it right, I
would definitely change it.

Nate Chapin

On 2014/06/03 20:53:10, Mounir Lamouri wrote: > https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/ManifestLoader.cpp > File Source/core/loader/ManifestLoader.cpp (right): > > https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/ManifestLoader.cpp#newcode23 ...

6 years, 6 months ago (2014-06-03 21:07:07 UTC) #20

On 2014/06/03 20:53:10, Mounir Lamouri wrote:
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> File Source/core/loader/ManifestLoader.cpp (right):
> 
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> Source/core/loader/ManifestLoader.cpp:23: ManifestLoader* manifestLoader = new
> ManifestLoader(request);
> On 2014/06/03 18:32:42, Nate Chapin wrote:
> > Should this be an OwnPtr<> that gets leaked if we actually start the load?
> > PingLoader::start uses a method similar to this.
> > 
> > startLoading() would need to return a boolean, but we wouldn't need to have
a
> > "delete this;" in every early-exit case in start loading.
> 
> I did something slightly different with OwnPtr<>(this) being instantiated on
the
> stack of ::startLoading() and ::notifyFinished() so |this| gets deleted when
> those functions are left unless .leakPtr() is called.
> 
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> Source/core/loader/ManifestLoader.cpp:51: if (!crossOriginMode.isNull()) {
> On 2014/06/03 18:32:42, Nate Chapin wrote:
> > Nit: { } unnecessary.
> 
> Done.
> 
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> File Source/core/loader/ManifestLoader.h (right):
> 
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> Source/core/loader/ManifestLoader.h:31: virtual void notifyFinished(Resource*)
> OVERRIDE;
> On 2014/06/03 18:32:42, Nate Chapin wrote:
> > Nit: should this be protected or private instead of public?
> 
> Good catch! :)
> 
>
https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
> File public/platform/WebManifestRequest.h (right):
> 
>
https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
> public/platform/WebManifestRequest.h:16: class WebManifestRequest {
> On 2014/06/03 18:32:42, Nate Chapin wrote:
> > I think part of my confusion about this patch is that this probably
shouldn't
> be
> > in public/platform/, as it's primarily a callback to notify high level code
> > about something blink is doing, not a low level API blink uses to do its
job.
> > 
> > The way we usually do this is to have a ManifestLoaderClient next to the
> > ManifestLoader in Source/core/loader/. ManifestLoaderClient is then
> implemented
> > in Source/web/ by a ManifestLoaderClientImpl, which notifies a
> WebManifestClient
> > in public/web/. :-/
> > 
> > At least, that's how I understand it. eseidel or one of the other
> public/OWNERS
> > might have a better idea.
> 
> I will ping eseidel. My design sounds simpler but if I'm not doing it right, I
> would definitely change it.

eseidel and I just chatted in person. Perhaps the best way to do this is one of
the following:
(1) Just expose the manifest url on WebFrame and don't make blink load the
manifest.
(2) Get rid of WebManifestRequest and put the necessary callbacks on
FrameLoaderClient (and WebFrameClient) instead. This probably involves
ManifestLoader needing to inherit from FrameDestructionObserver, but that's
pretty straightforward and means you can use existing classes to plumb your
callbacks through.

I'd lean toward (2), but I'd be fine with either

mlamouri (slow - plz ping)

On 2014/06/03 21:07:07, Nate Chapin wrote: > On 2014/06/03 20:53:10, Mounir Lamouri wrote: > > ...

6 years, 6 months ago (2014-06-03 21:49:02 UTC) #21

On 2014/06/03 21:07:07, Nate Chapin wrote:
> On 2014/06/03 20:53:10, Mounir Lamouri wrote:
> >
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> > File Source/core/loader/ManifestLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> > Source/core/loader/ManifestLoader.cpp:23: ManifestLoader* manifestLoader =
new
> > ManifestLoader(request);
> > On 2014/06/03 18:32:42, Nate Chapin wrote:
> > > Should this be an OwnPtr<> that gets leaked if we actually start the load?
> > > PingLoader::start uses a method similar to this.
> > > 
> > > startLoading() would need to return a boolean, but we wouldn't need to
have
> a
> > > "delete this;" in every early-exit case in start loading.
> > 
> > I did something slightly different with OwnPtr<>(this) being instantiated on
> the
> > stack of ::startLoading() and ::notifyFinished() so |this| gets deleted when
> > those functions are left unless .leakPtr() is called.
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> > Source/core/loader/ManifestLoader.cpp:51: if (!crossOriginMode.isNull()) {
> > On 2014/06/03 18:32:42, Nate Chapin wrote:
> > > Nit: { } unnecessary.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> > File Source/core/loader/ManifestLoader.h (right):
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/Source/core/loader/Manif...
> > Source/core/loader/ManifestLoader.h:31: virtual void
notifyFinished(Resource*)
> > OVERRIDE;
> > On 2014/06/03 18:32:42, Nate Chapin wrote:
> > > Nit: should this be protected or private instead of public?
> > 
> > Good catch! :)
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
> > File public/platform/WebManifestRequest.h (right):
> > 
> >
>
https://codereview.chromium.org/295063002/diff/60001/public/platform/WebManif...
> > public/platform/WebManifestRequest.h:16: class WebManifestRequest {
> > On 2014/06/03 18:32:42, Nate Chapin wrote:
> > > I think part of my confusion about this patch is that this probably
> shouldn't
> > be
> > > in public/platform/, as it's primarily a callback to notify high level
code
> > > about something blink is doing, not a low level API blink uses to do its
> job.
> > > 
> > > The way we usually do this is to have a ManifestLoaderClient next to the
> > > ManifestLoader in Source/core/loader/. ManifestLoaderClient is then
> > implemented
> > > in Source/web/ by a ManifestLoaderClientImpl, which notifies a
> > WebManifestClient
> > > in public/web/. :-/
> > > 
> > > At least, that's how I understand it. eseidel or one of the other
> > public/OWNERS
> > > might have a better idea.
> > 
> > I will ping eseidel. My design sounds simpler but if I'm not doing it right,
I
> > would definitely change it.
> 
> eseidel and I just chatted in person. Perhaps the best way to do this is one
of
> the following:
> (1) Just expose the manifest url on WebFrame and don't make blink load the
> manifest.

I want CORS support and it seems that I need Blink to make that happen.

> (2) Get rid of WebManifestRequest and put the necessary callbacks on
> FrameLoaderClient (and WebFrameClient) instead. This probably involves
> ManifestLoader needing to inherit from FrameDestructionObserver, but that's
> pretty straightforward and means you can use existing classes to plumb your
> callbacks through.
> 
> I'd lean toward (2), but I'd be fine with either

I will look into that.

mlamouri (slow - plz ping)

After talking to Nate offline, it's no longer clear what I need to change. I ...

6 years, 4 months ago (2014-07-28 11:44:35 UTC) #22

dcheng

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/ManifestLoaderTest.cpp File Source/core/loader/ManifestLoaderTest.cpp (right): https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/ManifestLoaderTest.cpp#newcode89 Source/core/loader/ManifestLoaderTest.cpp:89: document()->updateSecurityOrigin(SecurityOrigin::create(url)); I commented on this in an earlier review ...

6 years, 4 months ago (2014-07-28 17:08:58 UTC) #23

mlamouri (slow - plz ping)

6 years, 4 months ago (2014-08-19 16:14:18 UTC) #24

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
File Source/core/loader/ManifestLoaderTest.cpp (right):

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
Source/core/loader/ManifestLoaderTest.cpp:89:
document()->updateSecurityOrigin(SecurityOrigin::create(url));
On 2014/07/28 17:08:57, dcheng (OOO) wrote:
> I commented on this in an earlier review pass. Can we just use the normal
loader
> steps for loading a document rather than trying to emulate them?

Do you mean doing a loading via a mocked url and then ask the frame to load the
given url? That's a fairly involved process that most tests do via
FrameTestHelpers, which lives in web/tests/.

Note that the intent here is not to simulate the document loading but to simply
set up the document in order to test ManifestLoader. The class uses a very
limited information from the document (baseURL(), securityOrigin(), fetcher(),
encoding(), linkManifest()). I'm not really convinced that doing the entire
loading process would be the right thing to do here. The goal is to test that if
the document's url and security origin has the correct impact on ManifestLoader,
not to test if they are set properly when the frame is loaded.

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
Source/core/loader/ManifestLoaderTest.cpp:145:
document()->head()->appendChild(link);
On 2014/07/28 17:08:57, dcheng (OOO) wrote:
> This goes with my earlier comment. I don't think putting this logic in a
helper
> function makes this test clearer. We should just make 4 small test files and
> dump them into Source/web/test. The combined length of those 4 test files will
> probably be shorter than the length of this entire function, and it makes it
> much easier to see exactly what's being tested.

I agree. On the other hand, that requires to do a proper loading of the file
while this is only setting the document in its bare minimum to be usable in this
unit test.

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
Source/core/loader/ManifestLoaderTest.cpp:153:
blink::Platform::current()->unitTestSupport()->serveAsynchronousMockedRequests();
On 2014/07/28 17:08:57, dcheng (OOO) wrote:
> If you need this, you need this. Just ignore the presubmit error on this.
> jyasskin and I have some long-term plans on how to eliminate the need for
this.

Ok.

https://codereview.chromium.org/295063002/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/295063002/diff/120001/public/web/WebFrame.h#n...
public/web/WebFrame.h:681: virtual void requestManifest(WebManifestRequest&)
const = 0;
On 2014/07/28 17:08:58, dcheng (OOO) wrote:
> Please move this interface method to WebLocalFrame.h, as I don't think this
will
> ever be implemented for WebRemoteFrame.

Done.

dcheng

6 years, 4 months ago (2014-08-19 16:30:29 UTC) #25

mlamouri (slow - plz ping)

6 years, 4 months ago (2014-08-19 16:37:05 UTC) #26

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
File Source/core/loader/ManifestLoaderTest.cpp (right):

https://codereview.chromium.org/295063002/diff/120001/Source/core/loader/Mani...
Source/core/loader/ManifestLoaderTest.cpp:89:
document()->updateSecurityOrigin(SecurityOrigin::create(url));
On 2014/08/19 16:30:29, dcheng (OOO) wrote:
> On 2014/08/19 16:14:18, Mounir Lamouri wrote:
> > On 2014/07/28 17:08:57, dcheng (OOO) wrote:
> > > I commented on this in an earlier review pass. Can we just use the normal
> > loader
> > > steps for loading a document rather than trying to emulate them?
> > 
> > Do you mean doing a loading via a mocked url and then ask the frame to load
> the
> > given url? That's a fairly involved process that most tests do via
> > FrameTestHelpers, which lives in web/tests/.
> > 
> > Note that the intent here is not to simulate the document loading but to
> simply
> > set up the document in order to test ManifestLoader. The class uses a very
> > limited information from the document (baseURL(), securityOrigin(),
fetcher(),
> > encoding(), linkManifest()). I'm not really convinced that doing the entire
> > loading process would be the right thing to do here. The goal is to test
that
> if
> > the document's url and security origin has the correct impact on
> ManifestLoader,
> > not to test if they are set properly when the frame is loaded.
> 
> Many of these properties are normally set by the loading process. Why would it
> be better to bypass that for these tests?

The goal is to have unit tests for ManifestLoader so I want to have minimal
dependencies on anything else. The behaviour of the loading code is going to be
useless dependency that could just introduce noise. For example, if the loading
code is broken and doesn't set the security origin properly, this test would
break while the code it is testing would still be working properly.

It would make sense to load the document inside a WebView if that test was meant
to be an integration test because if the loading isn't working properly the
feature would indeed be broken.

mlamouri (slow - plz ping)

Nate and Adam, could you have a look? I applied comments that I got from ...

6 years, 4 months ago (2014-08-25 16:35:06 UTC) #27

Nate Chapin

Overall design seems reasonable. I only looked carefully at core/loader/, core/fetch/, and web/. A few ...

6 years, 4 months ago (2014-08-25 20:43:12 UTC) #28

mlamouri (slow - plz ping)

https://codereview.chromium.org/295063002/diff/200001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/295063002/diff/200001/Source/core/fetch/ResourceFetcher.cpp#newcode128 Source/core/fetch/ResourceFetcher.cpp:128: case Resource::Manifest: On 2014/08/25 20:43:11, Nate Chapin wrote: > ...

6 years, 3 months ago (2014-08-26 17:25:23 UTC) #29

https://codereview.chromium.org/295063002/diff/200001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/295063002/diff/200001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:128: case Resource::Manifest:
On 2014/08/25 20:43:11, Nate Chapin wrote:
> Is there any case loading a manifest will block the user? If not, this should
> probably be priority Low or VeryLow

For the moment, we expect to load the manifest only as needed. In the short
term, that means when the user wants to add the page to homescreen, or bookmark
it in some ways. I guess it can be considered as blocking the user.

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
File Source/core/loader/ManifestLoader.cpp (right):

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
Source/core/loader/ManifestLoader.cpp:40: FrameLoaderClient* client =
m_frame->loader().client();
On 2014/08/25 20:43:12, Nate Chapin wrote:
> Is this case possible at load start time?

Done.

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
Source/core/loader/ManifestLoader.cpp:44: Document* document =
m_frame->document();
On 2014/08/25 20:43:12, Nate Chapin wrote:
> I don't think this can't be null if there is a non-null FrameLoaderClient.

Done.

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
Source/core/loader/ManifestLoader.cpp:74: FrameLoaderClient* client =
m_frame->loader().client();
On 2014/08/25 20:43:12, Nate Chapin wrote:
> I don't think loads can complete when FrameLoaderClient is null, either.

Done.

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
File Source/core/loader/ManifestLoader.h (right):

https://codereview.chromium.org/295063002/diff/200001/Source/core/loader/Mani...
Source/core/loader/ManifestLoader.h:39: RefPtr<LocalFrame> m_frame;
On 2014/08/25 20:43:12, Nate Chapin wrote:
> Should ManifestLoader be able to keep the frame alive, or should it abort if
the
> frame is being destroyed?
> 
> My gut reaction is that ManifestLoader should inherit from
> FrameDestructionObserver and cancel the load on frame detach, but I'm willing
to
> be persuaded otherwise.

I tend to prefer to keep the frame alive but as you, I could be convinced
otherwise. With the short term plans regarding how we are going to use the
manifest, it will have no impact: we will only request it when the user will try
to add the tab to homescreen so we shouldn't expect the frame to go away.
Though, if it had to happen, we would want the request to finish so we can
proceed with the user's request.

In future plans, we might want to download the manifest in the background even
without a tab being open. We can't do that now given that we need a frame in
order to do the fetching.

Nate Chapin

Ok, The guts of the change look good. i'll defer to other folks for the ...

6 years, 3 months ago (2014-08-26 17:28:35 UTC) #30

mlamouri (slow - plz ping)

On 2014/08/26 17:28:35, Nate Chapin wrote: > Ok, The guts of the change look good. ...

6 years, 3 months ago (2014-08-26 17:30:17 UTC) #31

dcheng

I still think that if the code is going to call serveAsynchronousMockedRequests(), it may as ...

6 years, 3 months ago (2014-08-26 17:36:28 UTC) #32

mlamouri (slow - plz ping)

On 2014/08/26 17:36:28, dcheng (OOO) wrote: > I still think that if the code is ...

6 years, 3 months ago (2014-08-26 23:50:22 UTC) #33

dcheng

On 2014/08/26 at 23:50:22, mlamouri wrote: > On 2014/08/26 17:36:28, dcheng (OOO) wrote: > > ...

6 years, 3 months ago (2014-08-27 00:12:31 UTC) #34

On 2014/08/26 at 23:50:22, mlamouri wrote:
> On 2014/08/26 17:36:28, dcheng (OOO) wrote:
> > I still think that if the code is going to call
> > serveAsynchronousMockedRequests(), it may as well use the entire loader
> > machinery. It's already using the half that lives on the embedder side. I'm
also
> > really not a fan of assembling the test case programmatically like this
patch
> > does, since I find that it makes them harder to understand.
> 
> Given that serveAsynchronousMockedRequests() is part of WebUnitTestSupport
such as registerMockedURL() and registerMockedErrorURL(), it doesn't seem very
odd to use it, apart from the fact that it is meant to be deprecated. All of
that is part of a lower layer, from my understanding. What makes me
uncomfortable with using WebViewHelper is that it is part of a higher layer so I
would have to test something from core/ in web/. I don't think it is a good idea
to pull too many dependencies in unit tests and as long as we can keep them
self-contained, we should do that.
> 
> This said, I think we are going in circles here. Anyone else has an opinion?

Let's consider the two points separately.

I am strongly against the test cases being constructed programmatically like
this. It makes it more difficult for someone fixing/refactoring code to
understand what the test is doing. I would like to see the actual test case,
e.g. somewhere in the CORS404CrossOriginURL test case, I should see that the
test document is "<html><head><link rel="manifest"
href="my_favorite_manifest.json" crossorigin=""></head></html>". This was
probably not clear in my past replies, but this is my primary objection.

The other point is mocking out the loading machinery. By mocking it out, it's
true that the test loses the hard dependencies on the loader. But now, if
someone ever refactors how loads work or how link tags work, they have go and
fix this test too. None of these internal details about the loader or <link>
should really matter to this test, but they do now.

dcheng

(Also it looks like you have a bad rebase uploaded. There's a bunch of random ...

6 years, 3 months ago (2014-08-27 00:13:16 UTC) #35

kenneth.christiansen

kenneth.christiansen@gmail.com changed reviewers: + kenneth.christiansen@gmail.com

6 years, 3 months ago (2014-08-27 14:49:38 UTC) #36

kenneth.christiansen

https://codereview.chromium.org/295063002/diff/240001/Source/core/loader/ManifestLoader.cpp File Source/core/loader/ManifestLoader.cpp (right): https://codereview.chromium.org/295063002/diff/240001/Source/core/loader/ManifestLoader.cpp#newcode38 Source/core/loader/ManifestLoader.cpp:38: // When deleted, |selfDestruction| will delete |this|. Should be ...

6 years, 3 months ago (2014-08-27 14:49:38 UTC) #37

Mikhail

mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com

6 years, 3 months ago (2014-08-29 12:49:18 UTC) #38

Mikhail

https://codereview.chromium.org/295063002/diff/240001/Source/core/testing/URLTestHelpers.cpp File Source/core/testing/URLTestHelpers.cpp (right): https://codereview.chromium.org/295063002/diff/240001/Source/core/testing/URLTestHelpers.cpp#newcode56 Source/core/testing/URLTestHelpers.cpp:56: WebURLResponse response(fullURL); is this change Manifest-related? https://codereview.chromium.org/295063002/diff/240001/Source/modules/websockets/DOMWebSocketTest.cpp File Source/modules/websockets/DOMWebSocketTest.cpp ...

6 years, 3 months ago (2014-08-29 12:49:18 UTC) #39

mlamouri (slow - plz ping)

dcheng@, this is including all the changes you asked for. PTAL. https://codereview.chromium.org/295063002/diff/240001/Source/core/loader/FrameLoaderClient.h File Source/core/loader/FrameLoaderClient.h (right): ...

6 years, 3 months ago (2014-09-01 19:23:51 UTC) #40

dcheng

https://codereview.chromium.org/295063002/diff/280001/Source/core/loader/FrameLoaderClient.h File Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/295063002/diff/280001/Source/core/loader/FrameLoaderClient.h#newcode39 Source/core/loader/FrameLoaderClient.h:39: #include "public/platform/WebManifestError.h" // can't fwd declare enums. Nit: this ...

6 years, 3 months ago (2014-09-01 21:31:14 UTC) #41

mlamouri (slow - plz ping)

Comments applied. PTAL. https://codereview.chromium.org/295063002/diff/280001/Source/core/loader/FrameLoaderClient.h File Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/295063002/diff/280001/Source/core/loader/FrameLoaderClient.h#newcode39 Source/core/loader/FrameLoaderClient.h:39: #include "public/platform/WebManifestError.h" // can't fwd declare ...

6 years, 3 months ago (2014-09-01 22:49:14 UTC) #42

kenneth.christiansen

lgtm https://codereview.chromium.org/295063002/diff/300001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/295063002/diff/300001/Source/core/fetch/ResourceFetcher.cpp#newcode459 Source/core/fetch/ResourceFetcher.cpp:459: case Resource::Manifest: Link to http://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable ? https://codereview.chromium.org/295063002/diff/300001/Source/core/loader/ManifestLoader.h File ...

6 years, 3 months ago (2014-09-02 08:42:23 UTC) #43

dcheng

lgtm if comment is addressed. https://codereview.chromium.org/295063002/diff/300001/Source/web/WebLocalFrameImpl.cpp File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/295063002/diff/300001/Source/web/WebLocalFrameImpl.cpp#newcode1513 Source/web/WebLocalFrameImpl.cpp:1513: ManifestLoader::loadManifest(PassRefPtr<LocalFrame>(m_frame)); PassRefPtr has a ...

6 years, 3 months ago (2014-09-02 08:56:34 UTC) #44

Mike West

LGTM2, iff you add a test verifying the mixed content checking behavior.

6 years, 3 months ago (2014-09-02 12:50:05 UTC) #46

mlamouri (slow - plz ping)

Nate, could you stamp the Source/web/ part of this CL? It seems that you reviewed ...

6 years, 3 months ago (2014-09-02 15:24:40 UTC) #47

Should we close this one?

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages