Bug 42177 – [Chromium] Fix KURLGoogle and WEBKIT_IMPLEMENTATION for chromium multi-dll build

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

[Victor Wang]

Fix the following two items for chromium multi-dll build:
-. Update KURLGoogle decodeURLEscapeSequences to use googleurl public api so it does not access functions in url_canon_internal.
-. Fix WEBKIT_IMPLEMETATION in WebCommon.h so DllImport works as expected.

[Victor Wang]

Created attachment 61463 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 61463 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3497282

[Victor Wang]

(In reply to comment #2)
> Attachment 61463 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3497282

Dimitri,

The patch uses a new method in googleurl r138+ but the bot still pulls googleurl r137:
________ running 'svn update /mnt/git/webkit-chromium-ews/WebKit/chromium/googleurl --revision 137 --force' in '/mnt/git/webkit-chromium-ews/WebKit/chromium' At revision 137.

WebKit/chromium/DEPS has:
'chromium_rev': '51736'

Looks like I need to update chromium_rev to latest one, or at least the chromium revision that pulls the latest googleurl. Please confirm. Thanks!

[Victor Wang]

Created attachment 61465 [details]
New patch with rolling chromium deps to r52273

[WebKit Commit Bot]

Comment on attachment 61465 [details]
New patch with rolling chromium deps to r52273

Rejecting patch 61465 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20688 test cases.
security/block-test-no-port.html -> crashed

Exiting early after 1 failures. 17451 tests run.
608.59s total testing time

17450 test cases (99%) succeeded
1 test case (<1%) crashed
26 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3557001

[Adam Barth]

Comment on attachment 61465 [details]
New patch with rolling chromium deps to r52273

lets try that again

[Victor Wang]

(In reply to comment #6)
> (From update of attachment 61465 [details])
> lets try that again

Thanks! My patch should not affect this layout test and looks like the same test fails webkit buildbot: http://build.webkit.org/builders/Windows%20Release%20(Tests)/builds/1480/steps/layout-test/logs/stdio

[WebKit Commit Bot]

Comment on attachment 61465 [details]
New patch with rolling chromium deps to r52273

Clearing flags on attachment: 61465

Committed r63389: <http://trac.webkit.org/changeset/63389>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63389 might have broken Chromium Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63384
http://trac.webkit.org/changeset/63385
http://trac.webkit.org/changeset/63386
http://trac.webkit.org/changeset/63389
http://trac.webkit.org/changeset/63383

[David Levin]

Rolled out due to build break. https://bugs.webkit.org/show_bug.cgi?id=42311

You may to consider landing patches like this by hand so you'll be around for fallout. (The commit queue only builds the regular webkit for OSX and runs layout tests. This change is exclusively about  Chromium so the CQ provides no benefit for it.)

[David Levin]

See previous comment (bugzilla made me write something here).

[Victor Wang]

Created attachment 61705 [details]
New patch without chromium deps rolls (done in separate patch)

The chromium linux webkit break is due to the chromium deps roll. It is caused by changes between the time the patch ran on cr-linux EWS and the time it was actually landed.
Remove the chromium DEPS roll from this patch and did it separately: http://trac.webkit.org/changeset/63446.
Also update gyp so c4291 warning is disabled for chromium multi dll build.

[WebKit Review Bot]

Attachment 61705 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Victor Wang]

Created attachment 61708 [details]
Fix styles

[David Levin]

Comment on attachment 61708 [details]
Fix styles

r=me (as long as cr-linux passes). I'm not marking this as r+ so that ews cr-linux will run, but feel free to submit with "Reviewed by David Levin." as soon as that goes green on this attachment.

[Adam Barth]

> r=me (as long as cr-linux passes).

Sorry you have to work around this bug.  :(

[David Levin]

(In reply to comment #17)
> > r=me (as long as cr-linux passes).
> 
> Sorry you have to work around this bug.  :(

No worries. If it was a huge pain, I should fix it.  (It isn't a big deal.)

[WebKit Review Bot]

Attachment 61708 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3572043

[Victor Wang]

Comment on attachment 61708 [details]
Fix styles

Clearing flags on attachment: 61708

Committed r63516: <http://trac.webkit.org/changeset/63516>

[Victor Wang]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63516 might have broken Chromium Win Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63514
http://trac.webkit.org/changeset/63515
http://trac.webkit.org/changeset/63516
http://trac.webkit.org/changeset/63517
http://trac.webkit.org/changeset/63518