Bug 42177 - [Chromium] Fix KURLGoogle and WEBKIT_IMPLEMENTATION for chromium multi-dll build
Summary: [Chromium] Fix KURLGoogle and WEBKIT_IMPLEMENTATION for chromium multi-dll build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on: 42311
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-13 10:55 PDT by Victor Wang
Modified: 2010-07-15 23:25 PDT (History)
8 users (show)

See Also:


Attachments
Proposed Patch (6.84 KB, patch)
2010-07-13 20:46 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
New patch with rolling chromium deps to r52273 (7.31 KB, patch)
2010-07-13 21:31 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
New patch without chromium deps rolls (done in separate patch) (8.40 KB, patch)
2010-07-15 13:30 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Fix styles (8.40 KB, patch)
2010-07-15 13:34 PDT, Victor Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2010-07-13 10:55:40 PDT
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.
Comment 1 Victor Wang 2010-07-13 20:46:31 PDT
Created attachment 61463 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2010-07-13 20:54:15 PDT
Attachment 61463 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3497282
Comment 3 Victor Wang 2010-07-13 21:18:38 PDT
(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!
Comment 4 Victor Wang 2010-07-13 21:31:33 PDT
Created attachment 61465 [details]
New patch with rolling chromium deps to r52273
Comment 5 WebKit Commit Bot 2010-07-14 17:15:47 PDT
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
Comment 6 Adam Barth 2010-07-14 17:22:36 PDT
Comment on attachment 61465 [details]
New patch with rolling chromium deps to r52273

lets try that again
Comment 7 Victor Wang 2010-07-14 17:26:26 PDT
(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
Comment 8 WebKit Commit Bot 2010-07-14 19:01:31 PDT
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>
Comment 9 WebKit Commit Bot 2010-07-14 19:01:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 David Levin 2010-07-14 19:30:41 PDT
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.)
Comment 12 David Levin 2010-07-14 19:31:12 PDT
See previous comment (bugzilla made me write something here).
Comment 13 Victor Wang 2010-07-15 13:30:14 PDT
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.
Comment 14 WebKit Review Bot 2010-07-15 13:31:41 PDT
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.
Comment 15 Victor Wang 2010-07-15 13:34:27 PDT
Created attachment 61708 [details]
Fix styles
Comment 16 David Levin 2010-07-15 13:40:18 PDT
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.
Comment 17 Adam Barth 2010-07-15 13:42:01 PDT
> r=me (as long as cr-linux passes).

Sorry you have to work around this bug.  :(
Comment 18 David Levin 2010-07-15 13:47:51 PDT
(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.)
Comment 19 WebKit Review Bot 2010-07-15 17:04:16 PDT
Attachment 61708 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3572043
Comment 20 Victor Wang 2010-07-15 22:16:30 PDT
Comment on attachment 61708 [details]
Fix styles

Clearing flags on attachment: 61708

Committed r63516: <http://trac.webkit.org/changeset/63516>
Comment 21 Victor Wang 2010-07-15 22:16:54 PDT
All reviewed patches have been landed.  Closing bug.