Implement icon attribute for menuitem

Implement icon attribute for menuitem

Specification:
https://html.spec.whatwg.org/multipage/forms.html#attr-menuitem-icon

Link to "Intent to implement":
http://goo.gl/UtWWY8

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

[pals, 2015-01-02 11:57:16]

sanjoy.pal@samsung.com changed reviewers:
+ haraken@chromium.org

[pals, 2015-01-02 12:01:23]

tkent used to review menuitem related patches, but he is in leave. 
haraken, Could you please take a look? 
This patch is basically exposing the icon attribute of menuitem to chromium
side, so that chromium can download the icons and update the context menu.

[pals, 2015-01-02 12:07:47]

https://codereview.chromium.org/802233004/diff/20001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/802233004/diff/20001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:85: String icon =
menuItem->fastGetAttribute(iconAttr);
We need to resolve icon's url relative to the element as per spec. I'll update
this CL.

[pals, 2015-01-02 13:05:42]

https://codereview.chromium.org/802233004/diff/20001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/802233004/diff/20001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:85: String icon =
menuItem->fastGetAttribute(iconAttr);
On 2015/01/02 12:07:47, pals wrote:
> We need to resolve icon's url relative to the element as per spec. I'll update
> this CL.

Done.

[haraken, 2015-01-02 16:44:50]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org

[haraken, 2015-01-02 16:44:51]

I'm not familiar with the code...

keishi@: Would you mind taking a look?

[keishi, 2015-01-05 02:18:00]

LGTM

[pals, 2015-01-13 11:39:55]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-01-13 11:40:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/802233004/60001

[commit-bot: I haz the power, 2015-01-13 11:40:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-13 11:40:59]

Failed to apply patch for
LayoutTests/http/tests/misc/custom-context-menu-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         LayoutTests/http/tests/misc/custom-context-menu-expected.txt
  Copied LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt
-> LayoutTests/http/tests/misc/custom-context-menu-expected.txt
  patching file LayoutTests/http/tests/misc/custom-context-menu-expected.txt
  Hunk #1 FAILED at 3.
  Hunk #2 FAILED at 24.
  2 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/http/tests/misc/custom-context-menu-expected.txt.rej

Patch:   NR 
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt->LayoutTests/http/tests/misc/custom-context-menu-expected.txt
Index: LayoutTests/http/tests/misc/custom-context-menu-expected.txt
diff --git
a/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt
b/LayoutTests/http/tests/misc/custom-context-menu-expected.txt
similarity index 90%
copy from LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt
copy to LayoutTests/http/tests/misc/custom-context-menu-expected.txt
index
d286d4bb30de38f66a8f1384b92eb56f5b753f5f..c41b6bff893168371cfb01e4030f62868478f52f
100644
--- a/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt
+++ b/LayoutTests/http/tests/misc/custom-context-menu-expected.txt
@@ -3,7 +3,7 @@ Tests the custom context menu items.
 On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
 
 
-PASS itemsWithCustomContextMenu.length - items.length is 20
+PASS itemsWithCustomContextMenu.length - items.length is 21
 PASS itemsWithCustomContextMenu[0] is "Item1"
 PASS itemsWithCustomContextMenu[1] is "#Item2"
 PASS itemsWithCustomContextMenu[2] is "---------"
@@ -24,6 +24,7 @@ PASS itemsWithCustomContextMenu[16] is "Item13"
 PASS itemsWithCustomContextMenu[17] is "Item14"
 PASS itemsWithCustomContextMenu[18] is "*Item15"
 PASS itemsWithCustomContextMenu[19] is "Item16"
+PASS itemsWithCustomContextMenu[20] is
"Item17http://127.0.0.1:8000/misc/abc.png"
 PASS items1.length - items.length is 0
 PASS items2.length - items.length is 0
 PASS items3.length - items.length is 0

[pals, 2015-01-14 05:45:07]

Patchset #5 (id:80001) has been deleted

[pals, 2015-01-14 10:06:32]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-01-14 10:07:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/802233004/120001

[commit-bot: I haz the power, 2015-01-14 10:13:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-14 10:13:30]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[pals, 2015-01-14 11:42:38]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-01-14 11:43:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/802233004/120001

[commit-bot: I haz the power, 2015-01-14 11:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-14 11:50:14]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[pals, 2015-01-14 13:05:53]

sanjoy.pal@samsung.com changed reviewers:
+ chrishtr@chromium.org

[pals, 2015-01-14 13:07:32]

On 2015/01/14 11:50:14, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

@chrishtr, 
Need owners LGTM for the following files
Source/bindings/core/v8/custom/V8DevToolsHostCustom.cpp
Source/platform/ContextMenuItem.cpp
Source/platform/ContextMenuItem.h
Source/web/ContextMenuClientImpl.cpp

PTAL

[chrishtr, 2015-01-14 18:25:38]

I'll defer to haraken's review for these.

[haraken, 2015-01-14 23:26:26]

The implementation looks good.

What's the conclusion of the Intent-to-Implement for this? (Sorry, the thread
was too long to read :-)
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/MkEDloT-yu8

[pals, 2015-01-15 05:07:21]

On 2015/01/14 23:26:26, haraken wrote:
> The implementation looks good.
> 
> What's the conclusion of the Intent-to-Implement for this? (Sorry, the thread
> was too long to read :-)
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/MkEDloT-yu8

Intent was LGTMed. Implementation is almost complete. I am planning to ship
"contextmenu" and <menuitem>
once the "icon" attribute is completed.

Link to "Intent to implement":
http://goo.gl/UtWWY8

[haraken, 2015-01-15 05:09:43]

On 2015/01/15 05:07:21, pals wrote:
> On 2015/01/14 23:26:26, haraken wrote:
> > The implementation looks good.
> > 
> > What's the conclusion of the Intent-to-Implement for this? (Sorry, the
thread
> > was too long to read :-)
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/MkEDloT-yu8
> 
> Intent was LGTMed. Implementation is almost complete. I am planning to ship
> "contextmenu" and <menuitem>
> once the "icon" attribute is completed.
> 
> Link to "Intent to implement":
> http://goo.gl/UtWWY8

Thanks, LGTM.

[pals, 2015-01-19 05:16:41]

christr:
PTAL for owner's review.

[chrishtr, 2015-01-20 19:13:03]

lgtm

[pals, 2015-01-21 05:39:22]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-01-21 05:39:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/802233004/120001

[commit-bot: I haz the power, 2015-01-21 07:31:11]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188718

[tkent, 2015-02-02 00:32:54]

We need a test for icon attribute values with leading/trailing spaces.

https://html.spec.whatwg.org/multipage/forms.html#attr-menuitem-icon
> he attribute's value must contain a valid non-empty URL potentially surrounded
by spaces

https://html.spec.whatwg.org/multipage/infrastructure.html#valid-non-empty-ur...
> A string is a valid non-empty URL potentially surrounded by spaces if, after
stripping leading and trailing whitespace from it, it is a valid non-empty URL.