Bug 45898 – editing/selection/context-menu-on-text.html fails on chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[John Gregg]

new test added in http://trac.webkit.org/changeset/67626/ fails on chromium, being skipped.

[John Gregg]

Created attachment 67810 [details]
Patch

[John Gregg]

Committed r67632: <http://trac.webkit.org/changeset/67632>

[John Gregg]

Patch landed was just test expectations.  Bug still exists.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67632 might have broken GTK Linux 32-bit Release

[Hajime Morrita]

Created attachment 73585 [details]
Patch

[Hajime Morrita]

(In reply to comment #5)
> Created an attachment (id=73585) [details]
> Patch

The patch doesn't care about spellchecking.
I filed Bug 49366 for that case.

[Kent Tamura]

Comment on attachment 73585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73585&action=review

> WebKit/chromium/src/WebBindings.cpp:330
> +    for (size_t i = 0; i < data.size(); i++)

nit: we prefer ++i

> WebKitTools/DumpRenderTree/chromium/EventSender.cpp:741
> +    // These constant is based on Safari's context menu because tests are made for it.

"These constants are"?

> WebKitTools/DumpRenderTree/chromium/EventSender.cpp:770
> +    // Clears last context menu data because
> +    // we need to know if the context menu be requested after following mouse events.

Line breaking between "because" and "we" looks curious.  The first line is not so long and it can contain more words.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:449
> +    OwnPtr<WebContextMenuData> given(new WebContextMenuData(contextMenuData));
> +    m_lastContextMenuData.swap(given);

Why not m_lastContextMenuData = adoptPtr(new WebContextmenuData(contextMenuData)) ?

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:455
> +    OwnPtr<WebContextMenuData> empty;
> +    m_lastContextMenuData.swap(empty);

Why not m_lastContextMenuData.clear()?

[Hajime Morrita]

Created attachment 73694 [details]
Patch

[Hajime Morrita]

Hi Kent-san, thank you for reviewing!
I updated the patch to address your feedback.

> > WebKit/chromium/src/WebBindings.cpp:330
> > +    for (size_t i = 0; i < data.size(); i++)
> 
> nit: we prefer ++i
Fixed.

> 
> > WebKitTools/DumpRenderTree/chromium/EventSender.cpp:741
> > +    // These constant is based on Safari's context menu because tests are made for it.
> 
> "These constants are"?
Fixed.

> 
> > WebKitTools/DumpRenderTree/chromium/EventSender.cpp:770
> > +    // Clears last context menu data because
> > +    // we need to know if the context menu be requested after following mouse events.
> 
> Line breaking between "because" and "we" looks curious.  The first line is not so long and it can contain more words.
Fixed.

> 
> > WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:449
> > +    OwnPtr<WebContextMenuData> given(new WebContextMenuData(contextMenuData));
> > +    m_lastContextMenuData.swap(given);
> 
> Why not m_lastContextMenuData = adoptPtr(new WebContextmenuData(contextMenuData)) ?
Rewrote to use adoptPtr(). Thank you for telling this.

> 
> > WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:455
> > +    OwnPtr<WebContextMenuData> empty;
> > +    m_lastContextMenuData.swap(empty);
> 
> Why not m_lastContextMenuData.clear()?
Fixed.

[Kent Tamura]

Comment on attachment 73694 [details]
Patch

ok

[Hajime Morrita]

Committed r71886: <http://trac.webkit.org/changeset/71886>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71886 might have broken Chromium Win Release

[Marcus Bulach]

(In reply to comment #12)
> http://trac.webkit.org/changeset/71886 might have broken Chromium Win Release

We need to make a similar change to fix test_shell.
I've temporarily disabled this test again, see more details:
https://bugs.webkit.org/show_bug.cgi?id=45898

[Marcus Bulach]

(In reply to comment #13)
> (In reply to comment #12)
> > http://trac.webkit.org/changeset/71886 might have broken Chromium Win Release
> 
> We need to make a similar change to fix test_shell.
> I've temporarily disabled this test again, see more details:
> https://bugs.webkit.org/show_bug.cgi?id=45898

Sorry, this is the correct bug:
https://bugs.webkit.org/show_bug.cgi?id=49436

[Hajime Morrita]

Hi Bulach, thank you for file them!
I have one more upcoming DRT change.
So I'll port them to test_shell after land next one. 

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > http://trac.webkit.org/changeset/71886 might have broken Chromium Win Release
> > 
> > We need to make a similar change to fix test_shell.
> > I've temporarily disabled this test again, see more details:
> > https://bugs.webkit.org/show_bug.cgi?id=45898
> 
> Sorry, this is the correct bug:
> https://bugs.webkit.org/show_bug.cgi?id=49436