Implement the logic to find the correct <link> for Manifest in Document.

Implement the logic to find the correct <link> for Manifest in Document.

This is will then be use by WebFrame to get the data from the
HTMLLinkElement and fetch the ressource.

BUG=366145
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173823

[Inactive, 2014-05-08 20:13:29]

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4691: // The first link element with a manifest rel
must be used. Others are ignored.
May be a bit simpler to return early if head() is null:
HTMLHeadElement* head = this->head();
if (!head)
  return 0;
for (HTMLLinkElement* link = Traversal<HTMLLinkElement>::firstChild(*head);
link; link = Traversal<HTMLLinkElement>::nextSibling(*link)) {
  ...
}

[Inactive, 2014-05-08 20:17:12]

Looks good otherwise but I'll let Adam review.

[abarth-chromium, 2014-05-09 03:05:59]

LGTM with the test and the Internals function removed.

https://codereview.chromium.org/270283007/diff/1/Source/core/testing/Internal...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/270283007/diff/1/Source/core/testing/Internal...
Source/core/testing/Internals.cpp:2369: }
I'd skip adding this function.  We'll probably want to test this feature with a
WebViewTest rather than LayoutTests that poke internals.

[mlamouri (slow - plz ping), 2014-05-09 21:40:00]

On 2014/05/09 03:05:59, abarth wrote:
> LGTM with the test and the Internals function removed.
> 
>
https://codereview.chromium.org/270283007/diff/1/Source/core/testing/Internal...
> File Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/270283007/diff/1/Source/core/testing/Internal...
> Source/core/testing/Internals.cpp:2369: }
> I'd skip adding this function.  We'll probably want to test this feature with
a
> WebViewTest rather than LayoutTests that poke internals.

Adam, I've added some unit tests instead of the Internals. I assumed that this
should fine. I really want to make sure we test components by components given
that I'm not sure how much integration tests we will be able to do here.

PTAL :)

[abarth-chromium, 2014-05-10 00:36:57]

lgtm

Yay!  Nice unit test.  Much better than using internals.  :)

[commit-bot: I haz the power, 2014-05-10 00:38:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/270283007/20001

[Inactive, 2014-05-10 00:40:20]

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4691: // The first link element with a manifest rel
must be used. Others are ignored.
On 2014/05/08 20:13:29, Chris Dumez wrote:
> May be a bit simpler to return early if head() is null:
> HTMLHeadElement* head = this->head();
> if (!head)
>   return 0;
> for (HTMLLinkElement* link = Traversal<HTMLLinkElement>::firstChild(*head);
> link; link = Traversal<HTMLLinkElement>::nextSibling(*link)) {
>   ...
> }

Did you disagree with this proposal? It avoided calling head() twice (it is not
inlined) and also avoided 2 null checks if head() returns null.

[abarth-chromium, 2014-05-10 00:55:00]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-05-10 00:55:37]

Oh, sorry.  Yes, please address Chris' comment before landing.

[mlamouri (slow - plz ping), 2014-05-10 11:28:44]

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/270283007/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4691: // The first link element with a manifest rel
must be used. Others are ignored.
On 2014/05/10 00:40:20, Chris Dumez wrote:
> On 2014/05/08 20:13:29, Chris Dumez wrote:
> > May be a bit simpler to return early if head() is null:
> > HTMLHeadElement* head = this->head();
> > if (!head)
> >   return 0;
> > for (HTMLLinkElement* link = Traversal<HTMLLinkElement>::firstChild(*head);
> > link; link = Traversal<HTMLLinkElement>::nextSibling(*link)) {
> >   ...
> > }
> 
> Did you disagree with this proposal? It avoided calling head() twice (it is
not
> inlined) and also avoided 2 null checks if head() returns null.

Eh, sorry about that. I got focused on the tests and forgot to apply that
change. Done now.

[mlamouri (slow - plz ping), 2014-05-10 11:28:50]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-05-10 11:29:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/270283007/40001

[commit-bot: I haz the power, 2014-05-10 12:48:19]

Change committed as 173823