Allow the max url length to be overridden

Allow the max url length to be overridden

This is needed for WebView Classic compatibility, and since it shipped with KK it is needed to finish unforking.

There is no define to separate between Android WebView and
Chrome for Android which is why this is done with a setter
function instead of a define.

NOTRY=true
BUG=298495
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241065

[Kristian Monsen, 2013-12-10 21:27:59]

Did you think something along these lines?

[boliu, 2013-12-10 21:35:44]

Yep. Maybe consider wrapping the setter in OS_ANDROID.

https://codereview.chromium.org/112053002/diff/20001/android_webview/lib/main...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/112053002/diff/20001/android_webview/lib/main...
android_webview/lib/main/aw_main_delegate.cc:51: content::SetMaxURLChars(20 *
1024 * 1024);
Comment on why we need this.

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
content/public/common/url_utils.cc:14: static size_t g_maxUrlSize = 2 * 1024 *
1024;
g_max_url_size

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
content/public/common/url_utils.h:37: CONTENT_EXPORT void SetMaxURLChars(size_t
maxChars);
Maybe call this IncreaseMaxURLCharsForAndroidWebView or something like it. And
of course add comment to make clear that nothing else should call this?

[Kristian Monsen, 2013-12-10 21:42:06]

Updated based no feedback, PTAL.

https://codereview.chromium.org/112053002/diff/20001/android_webview/lib/main...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/112053002/diff/20001/android_webview/lib/main...
android_webview/lib/main/aw_main_delegate.cc:51: content::SetMaxURLChars(20 *
1024 * 1024);
On 2013/12/10 21:35:44, boliu wrote:
> Comment on why we need this.

Done.

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
content/public/common/url_utils.cc:14: static size_t g_maxUrlSize = 2 * 1024 *
1024;
On 2013/12/10 21:35:44, boliu wrote:
> g_max_url_size

Done.

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/20001/content/public/common/ur...
content/public/common/url_utils.h:37: CONTENT_EXPORT void SetMaxURLChars(size_t
maxChars);
On 2013/12/10 21:35:44, boliu wrote:
> Maybe call this IncreaseMaxURLCharsForAndroidWebView or something like it. And
> of course add comment to make clear that nothing else should call this?

Well, it can shrink it as well, so keeping the name unless you feel strong about
it. Added comment and wrapped in if defined(OS_ANDROID)

[boliu, 2013-12-10 21:46:47]

lgtm

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
Oh, just noticed, this doesn't need to be exported since all usage is under
content. But then the old constant was exported... I guess let content owners
decide.

[Kristian Monsen, 2013-12-10 22:01:24]

Please take a look as this is one of the final CL's blocking upstreaming of
Android WebView.

This is a continuation of https://codereview.chromium.org/84703003/ since that
approach was not good.

[Kristian Monsen, 2013-12-10 22:03:23]

Added tsepez for security.

[Tom Sepez, 2013-12-10 22:12:18]

Security LGTM, few nits.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.cc:14: static size_t g_max_url_size = 2 * 1024 *
1024;
nit: you can probably avoid introducing this static unless you're android.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.cc:39: g_maxUrlSize = maxChars;
nit: can you assert that you're android webview and not clank?

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
nit: not clear which process this needs to be called in, and whether it affects
other processes (it doesn't) for the case of clank. You may wind up with this
set to different values in different processes.

[boliu, 2013-12-10 22:21:59]

https://codereview.chromium.org/112053002/diff/40001/content/public/common/co...
File content/public/common/content_constants.h (left):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/co...
content/public/common/content_constants.h:50: CONTENT_EXPORT extern const size_t
kMaxURLChars;
Missed this one: Delete the actual variable in cc file.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.cc:39: g_maxUrlSize = maxChars;
On 2013/12/10 22:12:19, Tom Sepez wrote:
> nit: can you assert that you're android webview and not clank?

Not really, unless the assert is on something else completely unrelated, like
single process, or using the synchronous compositor.

How about rename this to SetMaxURLCharsForAndroidWebView?

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
On 2013/12/10 22:12:19, Tom Sepez wrote:
> nit: not clear which process this needs to be called in, and whether it
affects
> other processes (it doesn't) for the case of clank. You may wind up with this
> set to different values in different processes.

The implied usage is call this early in start up before forking child processes.
content::SetContentClient is another example of this.

Then of course we can make this explicit with a comment. Not sure if there is
anything to assert though.

[Jói, 2013-12-11 09:13:19]

Rubber stamp LGTM for //content/public with current set of comments addressed.

Note, when you add a reviewer to the review thread once it's started, please be
more explicit than just adding the reviewer to the Reviewers field in Rietveld.
It's much easier to notice a line in email that says e.g., "+joi for
//content/public" than it is to notice that from one email to another, I'm now
on the To: line.

Cheers,
Jói

[jam, 2013-12-11 17:23:31]

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
nit: usually getters have a Get prefix, which makes it clear what the method
does

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:38: // Should not be used exept by Android
WebView for backwards compatibility.
nit: except

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:39: CONTENT_EXPORT void SetMaxURLChars(size_t
maxChars);
nit: max_chars per style guide

[Kristian Monsen, 2013-12-11 22:05:41]

Should have addressed all the comments, PTAL.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/co...
File content/public/common/content_constants.h (left):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/co...
content/public/common/content_constants.h:50: CONTENT_EXPORT extern const size_t
kMaxURLChars;
On 2013/12/10 22:21:59, boliu wrote:
> Missed this one: Delete the actual variable in cc file.

Done.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.cc:14: static size_t g_max_url_size = 2 * 1024 *
1024;
On 2013/12/10 22:12:19, Tom Sepez wrote:
> nit: you can probably avoid introducing this static unless you're android.

I can but would prefer keeping the code simpler. If you or the other reviewers
strongly prefer that I can change.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.cc:39: g_maxUrlSize = maxChars;
On 2013/12/10 22:21:59, boliu wrote:
> On 2013/12/10 22:12:19, Tom Sepez wrote:
> > nit: can you assert that you're android webview and not clank?
> 
> Not really, unless the assert is on something else completely unrelated, like
> single process, or using the synchronous compositor.
> 
> How about rename this to SetMaxURLCharsForAndroidWebView?

I don't think we can assert. I can update the method name if that is the
consensus.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
File content/public/common/url_utils.h (right):

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
On 2013/12/11 17:23:32, jam wrote:
> nit: usually getters have a Get prefix, which makes it clear what the method
> does

Done. Should it also have the Google3 syntax for getters, get_max_url_chars?

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:33: CONTENT_EXPORT size_t MaxURLChars();
On 2013/12/10 22:21:59, boliu wrote:
> On 2013/12/10 22:12:19, Tom Sepez wrote:
> > nit: not clear which process this needs to be called in, and whether it
> affects
> > other processes (it doesn't) for the case of clank. You may wind up with
this
> > set to different values in different processes.
> 
> The implied usage is call this early in start up before forking child
processes.
> content::SetContentClient is another example of this.
> 
> Then of course we can make this explicit with a comment. Not sure if there is
> anything to assert though.

Done. Added a comment.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:38: // Should not be used exept by Android
WebView for backwards compatibility.
On 2013/12/11 17:23:32, jam wrote:
> nit: except

Done.

https://codereview.chromium.org/112053002/diff/40001/content/public/common/ur...
content/public/common/url_utils.h:39: CONTENT_EXPORT void SetMaxURLChars(size_t
maxChars);
On 2013/12/11 17:23:32, jam wrote:
> nit: max_chars per style guide

Done.

[Kristian Monsen, 2013-12-12 05:56:54]

Need two more owner lgtm:
jamesr for content/renderer
sky for content/browser

[sky, 2013-12-12 16:45:42]

Is there a reason for a setter? Should Android just have a different size?

https://codereview.chromium.org/112053002/diff/60001/content/public/common/ur...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/60001/content/public/common/ur...
content/public/common/url_utils.cc:37: #if defined (OS_ANDROID)
Order doesn't match header.

[Kristian Monsen, 2013-12-12 19:56:10]

Sky: there is no define to separate between android WebView and android chrome,
and we only want to set this for webview for security reasons.

I have a CL that says that in the function description and swaps the order of
the functions in the header file, but the uploads are getting 500 right now.

[sky, 2013-12-12 20:23:16]

Assuming you address my previous nit, LGTM

[jamesr, 2013-12-12 21:46:53]

How would this work in a multi-process environment? What would propagate the
limit to child processes?

[Kristian Monsen, 2013-12-12 21:52:39]

On 2013/12/12 21:46:53, jamesr wrote:
> How would this work in a multi-process environment? What would propagate the
> limit to child processes?

As described in the function comment, this should be called before any child
processes are started.

[jamesr, 2013-12-13 01:02:01]

It's not technically possible to share constants with child processes by setting
statics and forking.  Since this mechanism won't actually work for multi-process
users of content (i.e. nearly all of them except for this one case), can you
come up with a different way to plumb it?  I think we should either have a
setting that actually works or have one that is clearly restricted to Android
WebView, not have a setting that silently fails for other embeddings.

[Kristian Monsen, 2013-12-13 01:17:18]

On 2013/12/13 01:02:01, jamesr wrote:
> It's not technically possible to share constants with child processes by
setting
> statics and forking.  Since this mechanism won't actually work for
multi-process
> users of content (i.e. nearly all of them except for this one case), can you
> come up with a different way to plumb it?  I think we should either have a
> setting that actually works or have one that is clearly restricted to Android
> WebView, not have a setting that silently fails for other embeddings.

Would it be ok if we change the name to SetMaxURLCharsForAndroidWebView as Bo
suggested?

If not we are really running out of time for fixing this before the branching
deadline on Monday. We could use a command line flag or inject a define from the
gyp file if that is better?

[Kristian Monsen, 2013-12-13 01:43:11]

How does ContentClient work? It seems like that follows the same pattern with a
global and getter/setter?

I am certainly not an expert on this code, so any help to get this resolved
would be appreciated!

[jam, 2013-12-13 17:33:24]

On 2013/12/13 01:43:11, Kristian Monsen wrote:
> How does ContentClient work? It seems like that follows the same pattern with
a
> global and getter/setter?
> 
> I am certainly not an expert on this code, so any help to get this resolved
> would be appreciated!

You could CHECK(RenderProcessHost::run_renderer_in_process()) when the setter is
called. I don't know where we set single-process mode for android_webview, so
we'd need to make sure that's before this call of course.

[Kristian Monsen, 2013-12-13 20:25:36]

On 2013/12/13 17:33:24, jam wrote:
> On 2013/12/13 01:43:11, Kristian Monsen wrote:
> > How does ContentClient work? It seems like that follows the same pattern
with
> a
> > global and getter/setter?
> > 
> > I am certainly not an expert on this code, so any help to get this resolved
> > would be appreciated!
> 
> You could CHECK(RenderProcessHost::run_renderer_in_process()) when the setter
is
> called. I don't know where we set single-process mode for android_webview, so
> we'd need to make sure that's before this call of course.

Thanks jam, I have implemented this and I am testing it now. Sounds like a good
solution.

[Kristian Monsen, 2013-12-14 00:22:06]

On 2013/12/13 20:25:36, Kristian Monsen wrote:
> On 2013/12/13 17:33:24, jam wrote:
> > On 2013/12/13 01:43:11, Kristian Monsen wrote:
> > > How does ContentClient work? It seems like that follows the same pattern
> with
> > a
> > > global and getter/setter?
> > > 
> > > I am certainly not an expert on this code, so any help to get this
resolved
> > > would be appreciated!
> > 
> > You could CHECK(RenderProcessHost::run_renderer_in_process()) when the
setter
> is
> > called. I don't know where we set single-process mode for android_webview,
so
> > we'd need to make sure that's before this call of course.
> 
> Thanks jam, I have implemented this and I am testing it now. Sounds like a
good
> solution.

jam: This worked fine under testing, but as I discovered when uploading I am not
allowed to include content/public/browser/render_process_host.h from
content/public/common/url_utils.cc due to deps.

[jam, 2013-12-14 01:13:20]

On 2013/12/14 00:22:06, Kristian Monsen wrote:
> On 2013/12/13 20:25:36, Kristian Monsen wrote:
> > On 2013/12/13 17:33:24, jam wrote:
> > > On 2013/12/13 01:43:11, Kristian Monsen wrote:
> > > > How does ContentClient work? It seems like that follows the same pattern
> > with
> > > a
> > > > global and getter/setter?
> > > > 
> > > > I am certainly not an expert on this code, so any help to get this
> resolved
> > > > would be appreciated!
> > > 
> > > You could CHECK(RenderProcessHost::run_renderer_in_process()) when the
> setter
> > is
> > > called. I don't know where we set single-process mode for android_webview,
> so
> > > we'd need to make sure that's before this call of course.
> > 
> > Thanks jam, I have implemented this and I am testing it now. Sounds like a
> good
> > solution.
> 
> jam: This worked fine under testing, but as I discovered when uploading I am
not
> allowed to include content/public/browser/render_process_host.h from
> content/public/common/url_utils.cc due to deps.

oops, sorry I didn't think of that.

The only other alternative I can think of is having an ifdef that
android_webview_build in gypi creates. However I think that would be worse than
the current patch, because then code in content shouldn't know about embedders
including android_webview. Personally, I'm fine with the current patch. We can
comment it clearer to say that this is a per-process setting, and it's not
defined to call this in one process but not the others, if running in
multi-process mode.

[Kristian Monsen, 2013-12-14 02:13:40]

jam: I updated with a checking for single process command line flag. Would you
mind taking another look and lgtm if it looks good?

Thank you for the help so far!

[jam, 2013-12-16 17:40:36]

lgtm

https://codereview.chromium.org/112053002/diff/310001/content/public/common/u...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/310001/content/public/common/u...
content/public/common/url_utils.cc:44: // embedder
nit: why wrap at less than 80?

[Kristian Monsen, 2013-12-16 18:27:39]

Thank you! I will follow up with some testing and submit in an hour or so.

https://codereview.chromium.org/112053002/diff/310001/content/public/common/u...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/112053002/diff/310001/content/public/common/u...
content/public/common/url_utils.cc:44: // embedder
On 2013/12/16 17:40:36, jam wrote:
> nit: why wrap at less than 80?

Done.

[commit-bot: I haz the power, 2013-12-16 19:54:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/112053002/330001

[commit-bot: I haz the power, 2013-12-16 23:27:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/112053002/330001

[commit-bot: I haz the power, 2013-12-16 23:42:58]

Change committed as 241065