Bug 92931

Summary: [Chrome-Android] - Prepare apk tests for switch to checked in SDK.
Product: WebKit Reporter: Yaron Friedman <yfriedman>
Component: New BugsAssignee: Yaron Friedman <yfriedman>
Severity: Normal CC: abarth, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Description Flags
Patch none

Description Yaron Friedman 2012-08-01 18:48:16 PDT
[Chrome-Android] - Prepare apk tests for switch to checked in SDK.
Comment 1 Yaron Friedman 2012-08-01 18:49:15 PDT
Created attachment 155946 [details]
Comment 2 Yaron Friedman 2012-08-01 18:50:28 PDT
These are backwards compatible but add a few variable definitions so that ant builds will work after http://codereview.chromium.org/10830012/.
I've tested inside my chromium tree before and after the change.
Comment 3 Yaron Friedman 2012-08-01 18:52:34 PDT
As an aside: it seems like these rules would benefit from using build/apk_test.gypi as it would save a bunch of boilerplate. I'm guessing they aren't typically used in webkit?
Comment 4 Peter Beverloo 2012-08-02 06:24:01 PDT
Comment on attachment 155946 [details]

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

While this SGTM after the ChangeLog updates, it'll cause a build failure right now because the android_sdk_* variables are not yet available. You'll need to split your patch in three parts.. First land the envsetup_function.sh changes which introduce the variables, then roll Chromium into WebKit, then update WebKit with these changes, then roll WebKit into Chromium, then land the rest of your patch. Lovely, innit?

If you land the variables on the Chromium side and update your patch here, I'll make sure that anything up to rolling WebKit into Chromium is done before you come in tomorrow. You'll be able to pick it up after that.

As an aside, I think it's fine for WebKit to use gypi files in build/ (after all, we already use common.gypi too), but we can do that in a later patch, as this blocks your's on the Chromium side.

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please add a brief description of what's changing. Something among the lines of "Pass Android-specific gyp variables to the native test generator, avoiding any dependencies on environment variables during build time." would be perfect.

> Tools/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Comment 5 Yaron Friedman 2012-08-02 08:19:42 PDT
Created attachment 156090 [details]
Comment 6 WebKit Review Bot 2012-08-02 08:23:01 PDT
Attachment 156090 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 5 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yaron Friedman 2012-08-02 08:26:56 PDT
Created attachment 156092 [details]
Comment 8 Yaron Friedman 2012-08-03 09:18:27 PDT
Ok. Looks like chromium LKGR is passed my revision.  Can we roll chromium into webkit now? Do I just join #webkit and ask sheriffbot (http://www.chromium.org/developers/how-tos/webkit-gardening)
Comment 9 Adam Barth 2012-08-03 10:17:56 PDT
> Can we roll chromium into webkit now?


> Do I just join #webkit and ask sheriffbot (http://www.chromium.org/developers/how-tos/webkit-gardening)

Yep!  You say something like:

sheriffbot: roll-chromium-deps NNNNN
Comment 10 Yaron Friedman 2012-08-03 16:12:12 PDT
Ok, roll is in. Can we land this guy?
Comment 11 Adam Barth 2012-08-03 16:15:37 PDT
Comment on attachment 156092 [details]

Fine with me as long as Peter says it's ok.
Comment 12 WebKit Review Bot 2012-08-03 17:31:48 PDT
Comment on attachment 156092 [details]

Clearing flags on attachment: 156092

Committed r124676: <http://trac.webkit.org/changeset/124676>
Comment 13 WebKit Review Bot 2012-08-03 17:31:51 PDT
All reviewed patches have been landed.  Closing bug.