Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
368983004:
Implemented HandlePopupNavigation from tab_android. (Closed)
|
Created:
6 years, 5 months ago by Jitu( very slow this week) Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionImplemented HandlePopupNavigation from tab_android.
This patch handled opening of already blocked popups,
here it creates the webcontents and load the url in the
webcontents and invokes AddNewContents() of
ChromeWebContentsDelegateAndroid class so that it will notify
the application to add a new tab.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284015
Patch Set 1 #
Total comments: 6
Patch Set 2 : Formatted the patch #
Total comments: 1
Patch Set 3 : Formatted the patch #
Total comments: 5
Patch Set 4 : Exposed the api to convert NavigateParam to loadurlparam #
Total comments: 4
Patch Set 5 : Move api to correct place #Patch Set 6 : Rebase the patch #Patch Set 7 : Remove "BUG=" from commit msg #
Total comments: 10
Patch Set 8 : Fixed review comments #
Total comments: 4
Patch Set 9 : Handle dispostion type CURRENT_TAB #
Messages
Total messages: 43 (0 generated)
PTAL
I have to admit, I don't really know why things are the way they currently are here (i.e. why we have a special codepath for Android in PopupBlockerTabHelper::ShowBlockedPopup() that ends up in a NOTIMPLEMENTED instead of doing a regular navigation), so I'll only comment on style issues, and leave the remainder for the other reviewers. https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:184: params->source_contents, Fix alignment. `git cl format` can help you there. https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:220: const GURL& url, Alignment https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.h:150: content::WebContents* CreateTargetContents(const chrome::NavigateParams& params, Nit: Break this to fit into 80 columns, and fix alignment. https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.h:150: content::WebContents* CreateTargetContents(const chrome::NavigateParams& params, These don't need to be public. In fact, they can probably be moved into an anonymous namespace in the .cc file.
Fixed the review comments. PTAL https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.h:150: content::WebContents* CreateTargetContents(const chrome::NavigateParams& params, On 2014/07/04 14:06:29, Bernhard Bauer wrote: > Nit: Break this to fit into 80 columns, and fix alignment. Done.
https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:184: params->source_contents, On 2014/07/04 14:06:29, Bernhard Bauer wrote: > Fix alignment. `git cl format` can help you there. This is not fixed yet. Please make sure that you are looking at and addressing every single comment on a code review. Otherwise you're just wasting reviewers' time. https://codereview.chromium.org/368983004/diff/20001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/20001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:111: } // namespace One more space before //.
On 2014/07/04 16:46:20, Bernhard Bauer wrote: > https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... > File chrome/browser/android/tab_android.cc (right): > > https://codereview.chromium.org/368983004/diff/1/chrome/browser/android/tab_a... > chrome/browser/android/tab_android.cc:184: params->source_contents, > On 2014/07/04 14:06:29, Bernhard Bauer wrote: > > Fix alignment. `git cl format` can help you there. > > This is not fixed yet. Please make sure that you are looking at and addressing > every single comment on a code review. Otherwise you're just wasting reviewers' > time. > > https://codereview.chromium.org/368983004/diff/20001/chrome/browser/android/t... > File chrome/browser/android/tab_android.cc (right): > > https://codereview.chromium.org/368983004/diff/20001/chrome/browser/android/t... > chrome/browser/android/tab_android.cc:111: } // namespace > One more space before //. @Bernhard, I was having some problem in "git cl format" command due to some path setup. Thats why i have changed manually. Today i have fixed the "git cl format" issue in my system. And uploaded the patch again. Please have a look. Thanks again.
LGTM, but I would like one of the other reviewers to sanity-check (see my initial comment).
https://chromiumcodereview.appspot.com/368983004/diff/40001/chrome/browser/an... File chrome/browser/android/chrome_web_contents_delegate_android.cc (right): https://chromiumcodereview.appspot.com/368983004/diff/40001/chrome/browser/an... chrome/browser/android/chrome_web_contents_delegate_android.cc:326: if (was_blocked) Thanks for catching this! :) https://chromiumcodereview.appspot.com/368983004/diff/40001/chrome/browser/an... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/368983004/diff/40001/chrome/browser/an... chrome/browser/android/tab_android.cc:81: void LoadURLInContents(content::WebContents* target_contents, This looks more of a convert params. Almost like a NavigateParams -> LoadURLParams helper. Might be better written that way, so everyone who has to do the conversion can call it and we'd only have to update the one method. https://chromiumcodereview.appspot.com/368983004/diff/40001/chrome/browser/an... chrome/browser/android/tab_android.cc:234: void TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) { IIRC when this was originally written we didn't have much of this class, so it was abstract. Thanks for getting this working! It looks like we might not even need this method now. The only real call is from TabModelList::HandlePopupNavigation which uses the params->source_contents to figure out which TabAndroid to use at all... so we could just grab the delegate from that. We can't use chrome::Navigate() to do all of this work because much of it relies on a Browser* instance. But we could have a corresponding TabModelList::Navigate() until we figure out how to merge the two. Then you could move these callers to that (you should probably also move ContentViewCore's LoadURL call/other Android calls to that as well).
PTAL... https://codereview.chromium.org/368983004/diff/40001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/40001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:81: void LoadURLInContents(content::WebContents* target_contents, @David, When i have uploaded the first patch i have exposed this API LoadURLInContents(), after suggestion from "Bauer" i have changed this. As per your comments i have modified this API only like convert param so other can use. PTAL... Let me know if this you wanted. https://codereview.chromium.org/368983004/diff/40001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:234: void TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) { Yes i have tested it is working fine... I think till we have added the delegate mechanism in TabModelList lets have this implementation. And please suggest me so that i can try to add delegate mechanism and we will move all the codes here (TabModelList). Please Let me know on this.
https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:83: void TabAndroid::MakeLoadURLParam( Please move this method to a position corresponding to its position in the header. https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... chrome/browser/android/tab_android.h:150: void MakeLoadURLParam( This section is for methods called from Java via JNI. This method belongs further up, maybe after HasPrerenderedUrl()? Also, MakeLoadURLParams (note the 's' at the end), to match the struct.
PTAL https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:83: void TabAndroid::MakeLoadURLParam( On 2014/07/10 18:39:30, Bernhard Bauer wrote: > Please move this method to a position corresponding to its position in the > header. Done. https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/368983004/diff/60001/chrome/browser/android/t... chrome/browser/android/tab_android.h:150: void MakeLoadURLParam( On 2014/07/10 18:39:30, Bernhard Bauer wrote: > This section is for methods called from Java via JNI. This method belongs > further up, maybe after HasPrerenderedUrl()? > > Also, MakeLoadURLParams (note the 's' at the end), to match the struct. Done.
lgtm
On 2014/07/11 10:52:30, Bernhard Bauer wrote: > lgtm Thanks
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/368983004/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27487)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27497)
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/368983004/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27509)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27514)
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/368983004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/368983004/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/07/12 04:46:19, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) I am getting the below errors ... please help me out Missing LGTM from an OWNER for these files: chrome/browser/android/chrome_web_contents_delegate_android.cc chrome/browser/android/tab_android.cc chrome/browser/android/tab_android.h
On 2014/07/15 04:15:40, Jitu wrote: > On 2014/07/12 04:46:19, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium > > > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) > > I am getting the below errors ... please help me out > > Missing LGTM from an OWNER for these files: > chrome/browser/android/chrome_web_contents_delegate_android.cc > chrome/browser/android/tab_android.cc > chrome/browser/android/tab_android.h David / Yaron, it seems I'm not an OWNER in chrome/browser/android.
https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... chrome/browser/android/tab_android.cc:61: params.initiating_profile, Is this the right profile? What if we're opening an incognito tab? https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... chrome/browser/android/tab_android.cc:77: TabAndroid::AttachTabHelpers(target_contents); ChromeWebContentsDelegateAndroid::AddNewContents() seems to call this too. https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... chrome/browser/android/tab_android.cc:204: void TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) { Should we check disposition here? There are some dispositions we don't want to actually build a new WebContents for right? https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... chrome/browser/android/tab_android.cc:207: GURL url = params->url; GURL url(params->url)? https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... chrome/browser/android/tab_android.cc:259: load_url_params->is_renderer_initiated = params->is_renderer_initiated; Can we just pull this line out of the if block? Won't it always be the same?
On 2014/07/15 20:34:35, David Trainor wrote: > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > File chrome/browser/android/tab_android.cc (right): > > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > chrome/browser/android/tab_android.cc:61: params.initiating_profile, > Is this the right profile? What if we're opening an incognito tab? > > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > chrome/browser/android/tab_android.cc:77: > TabAndroid::AttachTabHelpers(target_contents); > ChromeWebContentsDelegateAndroid::AddNewContents() seems to call this too. > > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > chrome/browser/android/tab_android.cc:204: void > TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) { > Should we check disposition here? There are some dispositions we don't want to > actually build a new WebContents for right? > > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > chrome/browser/android/tab_android.cc:207: GURL url = params->url; > GURL url(params->url)? > > https://chromiumcodereview.appspot.com/368983004/diff/120001/chrome/browser/a... > chrome/browser/android/tab_android.cc:259: > load_url_params->is_renderer_initiated = params->is_renderer_initiated; > Can we just pull this line out of the if block? Won't it always be the same? Also please let me know when you're going to land so I can get started on a downstream patch to migrate to this code. Thanks!
PTAL... https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... chrome/browser/android/tab_android.cc:61: params.initiating_profile, On 2014/07/15 20:34:35, David Trainor wrote: > Is this the right profile? What if we're opening an incognito tab? We are assuming that params.initiating_profile gives the correct profile. For incognito tab i have done changes. PTAL... https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... chrome/browser/android/tab_android.cc:77: TabAndroid::AttachTabHelpers(target_contents); On 2014/07/15 20:34:35, David Trainor wrote: > ChromeWebContentsDelegateAndroid::AddNewContents() seems to call this too. Done. https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... chrome/browser/android/tab_android.cc:204: void TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) { On 2014/07/15 20:34:35, David Trainor wrote: > Should we check disposition here? There are some dispositions we don't want to > actually build a new WebContents for right? Done. PTAL https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... chrome/browser/android/tab_android.cc:207: GURL url = params->url; On 2014/07/15 20:34:35, David Trainor wrote: > GURL url(params->url)? Done. https://codereview.chromium.org/368983004/diff/120001/chrome/browser/android/... chrome/browser/android/tab_android.cc:259: load_url_params->is_renderer_initiated = params->is_renderer_initiated; On 2014/07/15 20:34:35, David Trainor wrote: > Can we just pull this line out of the if block? Won't it always be the same? Done.
I'm okay with this, but I just remembered that Yusuf might be relying on the way this method currently functions for his work. Giving him a chance to chime in/get a heads up on this change. yusufo@: Will this break your stuff? Let me know if you're okay with it and we can land it :). https://chromiumcodereview.appspot.com/368983004/diff/140001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/368983004/diff/140001/chrome/browser/a... chrome/browser/android/tab_android.cc:207: if (params->disposition != SUPPRESS_OPEN && there is a dcheck for params->disposition != CURRENT_TAB inside AddNewContents() IIRC. If it is CURRENT_TAB should we just call LoadURLWithParams on the current web_contents() here? https://chromiumcodereview.appspot.com/368983004/diff/140001/chrome/browser/a... chrome/browser/android/tab_android.cc:269: load_url_params->is_renderer_initiated = params->is_renderer_initiated; fix indentation
Changes done as per suggested. PTAL https://codereview.chromium.org/368983004/diff/140001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/368983004/diff/140001/chrome/browser/android/... chrome/browser/android/tab_android.cc:207: if (params->disposition != SUPPRESS_OPEN && On 2014/07/16 15:29:24, David Trainor wrote: > there is a dcheck for params->disposition != CURRENT_TAB inside AddNewContents() > IIRC. If it is CURRENT_TAB should we just call LoadURLWithParams on the current > web_contents() here? Done. https://codereview.chromium.org/368983004/diff/140001/chrome/browser/android/... chrome/browser/android/tab_android.cc:269: load_url_params->is_renderer_initiated = params->is_renderer_initiated; On 2014/07/16 15:29:23, David Trainor wrote: > fix indentation Done.
lgtm thanks!!
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/368983004/160001
Message was sent while issue was closed.
Change committed as 284015 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
