new test added in http://trac.webkit.org/changeset/67626/ fails on chromium, being skipped.
Created attachment 67810 [details] Patch
Committed r67632: <http://trac.webkit.org/changeset/67632>
Patch landed was just test expectations. Bug still exists.
http://trac.webkit.org/changeset/67632 might have broken GTK Linux 32-bit Release
Created attachment 73585 [details] Patch
(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.
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()?
Created attachment 73694 [details] Patch
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.
Comment on attachment 73694 [details] Patch ok
Committed r71886: <http://trac.webkit.org/changeset/71886>
http://trac.webkit.org/changeset/71886 might have broken Chromium Win Release
(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
(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
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