Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10696072:
Fix web-intent-related unit_tests linking issues on Android. (Closed)
|
Created:
8 years, 5 months ago by Philippe Modified:
8 years, 5 months ago CC:
chromium-reviews, marja+watch_chromium.org, Avi (use Gerrit), ajwong+watch_chromium.org, creis+watch_chromium.org, brettw-cc_chromium.org, James Hawkins Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix web-intent-related unit_tests linking issues on Android.
This disables the build of browser/ui/intent.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146118
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Address Ben's comment #
Messages
Total messages: 14 (0 generated)
+ Ben and Nico.
https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller.h (right): https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.h:9: #if defined(OS_ANDROID) looks like you're not building this file, which implies to me that code that you are building is including this file, and that's causing you grief. what code is including this file?
https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller.h (right): https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.h:9: #if defined(OS_ANDROID) On 2012/07/09 15:33:34, Ben Goodger (Google) wrote: > looks like you're not building this file, which implies to me that code that you > are building is including this file, and that's causing you grief. > > what code is including this file? I see at least these files explicitly including this header: chrome/browser/ui/browser.cc chrome/browser/ui/extensions/shell_window.cc chrome/browser/ui/gtk/web_intent_picker_gtk.cc chrome/browser/ui/intents/web_intent_picker_controller.cc chrome/browser/ui/tab_contents/tab_contents.cc My concern is mainly about browser.cc and tab_contents.cc. I think that the current approach introduces less #ifdefs than the other one which would be to include #ifdefs in these two files both at the include and usage levels. What do you think?
Are you building any of those files? On Mon, Jul 9, 2012 at 9:22 AM, <pliard@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10696072/diff/** > 8002/chrome/browser/ui/**intents/web_intent_picker_**controller.h<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h> > File chrome/browser/ui/intents/web_**intent_picker_controller.h (right): > > https://chromiumcodereview.**appspot.com/10696072/diff/** > 8002/chrome/browser/ui/**intents/web_intent_picker_**controller.h#newcode9<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h#newcode9> > chrome/browser/ui/intents/web_**intent_picker_controller.h:9: #if > defined(OS_ANDROID) > On 2012/07/09 15:33:34, Ben Goodger (Google) wrote: > >> looks like you're not building this file, which implies to me that >> > code that you > >> are building is including this file, and that's causing you grief. >> > > what code is including this file? >> > > I see at least these files explicitly including this header: > chrome/browser/ui/browser.cc > chrome/browser/ui/extensions/**shell_window.cc > chrome/browser/ui/gtk/web_**intent_picker_gtk.cc > chrome/browser/ui/intents/web_**intent_picker_controller.cc > chrome/browser/ui/tab_**contents/tab_contents.cc > > My concern is mainly about browser.cc and tab_contents.cc. I think that > the current approach introduces less #ifdefs than the other one which > would be to include #ifdefs in these two files both at the include and > usage levels. > > What do you think? > > https://chromiumcodereview.**appspot.com/10696072/<https://chromiumcodereview... >
On 2012/07/09 16:44:34, Ben Goodger (Google) wrote: > Are you building any of those files? > > > On Mon, Jul 9, 2012 at 9:22 AM, <mailto:pliard@chromium.org> wrote: > > > > > https://chromiumcodereview.**appspot.com/10696072/diff/** > > > 8002/chrome/browser/ui/**intents/web_intent_picker_**controller.h<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h> > > File chrome/browser/ui/intents/web_**intent_picker_controller.h (right): > > > > https://chromiumcodereview.**appspot.com/10696072/diff/** > > > 8002/chrome/browser/ui/**intents/web_intent_picker_**controller.h#newcode9<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h#newcode9> > > chrome/browser/ui/intents/web_**intent_picker_controller.h:9: #if > > defined(OS_ANDROID) > > On 2012/07/09 15:33:34, Ben Goodger (Google) wrote: > > > >> looks like you're not building this file, which implies to me that > >> > > code that you > > > >> are building is including this file, and that's causing you grief. > >> > > > > what code is including this file? > >> > > > > I see at least these files explicitly including this header: > > chrome/browser/ui/browser.cc > > chrome/browser/ui/extensions/**shell_window.cc > > chrome/browser/ui/gtk/web_**intent_picker_gtk.cc > > chrome/browser/ui/intents/web_**intent_picker_controller.cc > > chrome/browser/ui/tab_**contents/tab_contents.cc > > > > My concern is mainly about browser.cc and tab_contents.cc. I think that > > the current approach introduces less #ifdefs than the other one which > > would be to include #ifdefs in these two files both at the include and > > usage levels. > > > > What do you think? > > > > > https://chromiumcodereview.**appspot.com/10696072/%3Chttps://chromiumcoderevi...> > > We don't build browser.cc indeed but we do build tab_contents.cc and have Android-related changes in it.
Is there still an ENABLE_INTENTS or was that deprecated? Can't you just guard in tab_contents.cc? On Mon, Jul 9, 2012 at 10:10 AM, <pliard@chromium.org> wrote: > On 2012/07/09 16:44:34, Ben Goodger (Google) wrote: > >> Are you building any of those files? >> > > > On Mon, Jul 9, 2012 at 9:22 AM, <mailto:pliard@chromium.org> wrote: >> > > > >> > https://chromiumcodereview.**a**ppspot.com/10696072/diff/**<http://appspot.co... >> > >> > > 8002/chrome/browser/ui/****intents/web_intent_picker_****controller.h< > https://**chromiumcodereview.appspot.**com/10696072/diff/8002/chrome/** > browser/ui/intents/web_intent_**picker_controller.h<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h> > > > >> > File chrome/browser/ui/intents/web_****intent_picker_controller.h >> (right): >> > >> > https://chromiumcodereview.**a**ppspot.com/10696072/diff/**<http://appspot.co... >> > >> > > 8002/chrome/browser/ui/****intents/web_intent_picker_**** > controller.h#newcode9<https://**chromiumcodereview.appspot.** > com/10696072/diff/8002/chrome/**browser/ui/intents/web_intent_** > picker_controller.h#newcode9<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h#newcode9> > > > >> > chrome/browser/ui/intents/web_****intent_picker_controller.h:**9: #if >> >> > defined(OS_ANDROID) >> > On 2012/07/09 15:33:34, Ben Goodger (Google) wrote: >> > >> >> looks like you're not building this file, which implies to me that >> >> >> > code that you >> > >> >> are building is including this file, and that's causing you grief. >> >> >> > >> > what code is including this file? >> >> >> > >> > I see at least these files explicitly including this header: >> > chrome/browser/ui/browser.cc >> > chrome/browser/ui/extensions/****shell_window.cc >> > chrome/browser/ui/gtk/web_****intent_picker_gtk.cc >> > chrome/browser/ui/intents/web_****intent_picker_controller.cc >> > chrome/browser/ui/tab_****contents/tab_contents.cc >> >> > >> > My concern is mainly about browser.cc and tab_contents.cc. I think that >> > the current approach introduces less #ifdefs than the other one which >> > would be to include #ifdefs in these two files both at the include and >> > usage levels. >> > >> > What do you think? >> > >> > >> > > https://chromiumcodereview.**a**ppspot.com/10696072/%3Chttps:/** > /chromiumcodereview.appspot.**com/10696072/<http://appspot.com/10696072/%3Chttps://chromiumcodereview.appspot.com/10696072/> > > > >> > >> > > We don't build browser.cc indeed but we do build tab_contents.cc and have > Android-related changes in it. > > https://chromiumcodereview.**appspot.com/10696072/<https://chromiumcodereview... >
On 2012/07/09 17:52:10, Ben Goodger (Google) wrote: > Is there still an ENABLE_INTENTS or was that deprecated? Can't you just > guard in tab_contents.cc? > > > On Mon, Jul 9, 2012 at 10:10 AM, <mailto:pliard@chromium.org> wrote: > > > On 2012/07/09 16:44:34, Ben Goodger (Google) wrote: > > > >> Are you building any of those files? > >> > > > > > > On Mon, Jul 9, 2012 at 9:22 AM, <mailto:pliard@chromium.org> wrote: > >> > > > > > > >> > > https://chromiumcodereview.**a**ppspot.com/10696072/diff/**%3Chttp://appspot....> > >> > > >> > > > > 8002/chrome/browser/ui/****intents/web_intent_picker_****controller.h< > > https://**chromiumcodereview.appspot.**com/10696072/diff/8002/chrome/** > > > browser/ui/intents/web_intent_**picker_controller.h<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h> > > > > > > >> > File chrome/browser/ui/intents/web_****intent_picker_controller.h > >> (right): > >> > > >> > > https://chromiumcodereview.**a**ppspot.com/10696072/diff/**%3Chttp://appspot....> > >> > > >> > > > > 8002/chrome/browser/ui/****intents/web_intent_picker_**** > > controller.h#newcode9<https://**chromiumcodereview.appspot.** > > com/10696072/diff/8002/chrome/**browser/ui/intents/web_intent_** > > > picker_controller.h#newcode9<https://chromiumcodereview.appspot.com/10696072/diff/8002/chrome/browser/ui/intents/web_intent_picker_controller.h#newcode9> > > > > > > >> > chrome/browser/ui/intents/web_****intent_picker_controller.h:**9: #if > >> > >> > defined(OS_ANDROID) > >> > On 2012/07/09 15:33:34, Ben Goodger (Google) wrote: > >> > > >> >> looks like you're not building this file, which implies to me that > >> >> > >> > code that you > >> > > >> >> are building is including this file, and that's causing you grief. > >> >> > >> > > >> > what code is including this file? > >> >> > >> > > >> > I see at least these files explicitly including this header: > >> > chrome/browser/ui/browser.cc > >> > chrome/browser/ui/extensions/****shell_window.cc > >> > chrome/browser/ui/gtk/web_****intent_picker_gtk.cc > >> > chrome/browser/ui/intents/web_****intent_picker_controller.cc > >> > chrome/browser/ui/tab_****contents/tab_contents.cc > >> > >> > > >> > My concern is mainly about browser.cc and tab_contents.cc. I think that > >> > the current approach introduces less #ifdefs than the other one which > >> > would be to include #ifdefs in these two files both at the include and > >> > usage levels. > >> > > >> > What do you think? > >> > > >> > > >> > > > > https://chromiumcodereview.**a**ppspot.com/10696072/%253Chttps:/** > > > /chromiumcodereview.appspot.**com/10696072/<http://appspot.com/10696072/%3Chttps://chromiumcodereview.appspot.com/10696072/> > > > > > > >> > > >> > > > > We don't build browser.cc indeed but we do build tab_contents.cc and have > > Android-related changes in it. > > > > > https://chromiumcodereview.**appspot.com/10696072/%3Chttps://chromiumcoderevi...> > > Adding #ifdefs in tab_contents.cc when instantiating WebIntentPickerController does work too indeed. I did not notice initially but we have a similar change internally. I fixed that in my last patch set.
lgtm
lgtm The intents folks removed that gyp flag. They've said they'll work on intents for Android but haven't yet. Since this is causing us problems now, I think in order to make progress we'll have to exclude it on Android and circle back later. cc'ing jhawkins as FYI
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10696072/18001
Try job failure for 10696072-18001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10696072/18001
Change committed as 146118 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
