[Downloads] Add context parameter to help center article URL.

[Downloads] Add context parameter to help center article URL.

Interrupted downloads expose a "Learn more" URL which aims to help users
with resolving problems with their downloads. Add a context parameter so
that the browser can indicate the interrupt reason when opening the
page.

R=rdsmith@chromium.org
BUG=477418
Committed: https://crrev.com/44aee8794af1c4dbbf29caa7708da78f6b4e7fcf
Cr-Commit-Position: refs/heads/master@{#326367}

[asanka, 2015-04-21 15:50:04]

Hold off on review until we clarify how we should use the ctx= parameter.

[Randy Smith (Not in Mondays), 2015-04-21 17:28:37]

Two high level questions:
* Have you confirmed that this won't cause any problem to users while the help
center articles are being updated?  (I.e. that they'll get the old one until the
help center is updated to use the new format.)

* Do we have appropriate warnings in the source definition for the download
interrupt reasons so that we don't change the symbolic to integer mappings in
the future and completely confuse people with regard to the help center
articles?  My guess is that it's already tied to histograms, but this is yet
another thing pinning down "Don't change these numbers, really."

With those two comments and the comment below, LGTM.

https://codereview.chromium.org/1084123004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_commands_unittest.cc (right):

https://codereview.chromium.org/1084123004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_commands_unittest.cc:163:
EXPECT_EQ("p=ui_download_errors&ctx=22", learn_more_url.query());
I'd rather compare against the download interrupt reason casted to an integer. 
We don't want this test to be dependent on the reason -> integer mappings, even
if we need to keep those fixed so that we don't mess up the help articles.

[asanka, 2015-04-22 16:56:16]

On 2015/04/21 at 17:28:37, rdsmith wrote:
> Two high level questions:
> * Have you confirmed that this won't cause any problem to users while the help
center articles are being updated?  (I.e. that they'll get the old one until the
help center is updated to use the new format.)

Yup. The ctx= parameter doesn't affect the page being displayed. I.e. even now
with the context parameter, users will see the same page.

> * Do we have appropriate warnings in the source definition for the download
interrupt reasons so that we don't change the symbolic to integer mappings in
the future and completely confuse people with regard to the help center
articles?  My guess is that it's already tied to histograms, but this is yet
another thing pinning down "Don't change these numbers, really."

The interrupt reasons have warnings on them about UMA and persistence:

 // Note that the embedder is welcome to persist these values across
 // invocations of the browser, and possibly across browser versions.
 // Thus individual errors may be deprecated and new errors added, but
 // the values of particular errors should not be changed.

> With those two comments and the comment below, LGTM.

Thanks!

[asanka, 2015-04-22 16:58:12]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-04-22 16:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1084123004/#ps20001
(title: " ")

[asanka, 2015-04-22 16:58:47]

https://codereview.chromium.org/1084123004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_commands_unittest.cc (right):

https://codereview.chromium.org/1084123004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_commands_unittest.cc:163:
EXPECT_EQ("p=ui_download_errors&ctx=22", learn_more_url.query());
On 2015/04/21 at 17:28:37, rdsmith wrote:
> I'd rather compare against the download interrupt reason casted to an integer.
 We don't want this test to be dependent on the reason -> integer mappings, even
if we need to keep those fixed so that we don't mess up the help articles.

Done.

[commit-bot: I haz the power, 2015-04-22 17:00:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1084123004/20001

[commit-bot: I haz the power, 2015-04-22 20:15:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-22 20:16:16]

Patchset 2 (id:??) landed as
https://crrev.com/44aee8794af1c4dbbf29caa7708da78f6b4e7fcf
Cr-Commit-Position: refs/heads/master@{#326367}