Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706193002:
Expose final download URL (actual url after redirects) in the extension API. (Closed)
|
Created:
4 years, 10 months ago by mharanczyk Modified:
4 years, 5 months ago CC:
asanka, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose final download URL (actual url after redirects) in the extension API.
BUG=620630
Committed: https://crrev.com/235b8cbc6cff62240ea0628fc0d1c5ba4bec38e8
Cr-Commit-Position: refs/heads/master@{#404619}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase. #Patch Set 3 : Address reviewers' comments. #
Total comments: 12
Patch Set 4 : IDL doc update. #
Total comments: 7
Patch Set 5 : Address another batch of review comments. #
Total comments: 2
Patch Set 6 : Reworded comment to better match what the code does. #Patch Set 7 : Rebase #Patch Set 8 : Additional documentation. #
Total comments: 6
Patch Set 9 : Address another bunch of comments. #Patch Set 10 : Rebase #
Messages
Total messages: 37 (8 generated)
mharanczyk@opera.com changed reviewers: + asanka@chromium.org, asargent@chromium.org, benjhayden@chromium.org
Hi all, could you take a look at those changes and tell me if you fine with exposing additional field in extensions downloads api to expose information about actual download url (after all redirects)?
Ideally, the native API should be consistent and any mapping that we need to do between concepts in native vs. the extensions API should be done in downloads_api.cc. I.e. in DownloadQuery, instead of URL and FINAL_URL to refer to DownloadItem::GetOriginalUrl and DownloadItem::GetUrl respectively, we should be using ORIGINAL_URL and URL. Then encapsulate the mapping between 'url' and 'finalUrl' in downloads_api.cc. Other than that, no objections on my part. I'll stamp once benjhayden approves. https://codereview.chromium.org/1706193002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1706193002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_query.cc:123: static std::string GetFinalUrl(const DownloadItem& item) { "static" unnecessary for functions defined within an anonymous namespace.
nit: can you create a bug entry for this and link to it with BUG= in the CL description?
Ping! benjhayden@ could you please chime in here since you are the one calling the shots? I will make already requested corrections others asked for only if you are generally ok with this cl.
On 2016/05/16 at 13:14:39, mharanczyk wrote: > Ping! > benjhayden@ could you please chime in here since you are the one calling the shots? I will make already requested corrections others asked for only if you are generally ok with this cl. Sorry I missed this CL! Please IM me if I don't respond after a couple days! lgtm generally, but I agree with asanka's and asargent's requests. Also, would it be possible to write a test in downloads_api_browsertest involving a redirect so that url != finalUrl?
Description was changed from ========== Expose final download URL (actual url after redirects) in the extension API. ========== to ========== Expose final download URL (actual url after redirects) in the extension API. BUG=620630 ==========
The CQ bit was checked by mharanczyk@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706193002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've updated CL with all requested changes, please take a look again.
lgtm
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:138: // URL that this download started from (before any redirects). nit: in the autogenerated documentation at https://developer.chrome.com/extensions/downloads#type-DownloadItem, this will read as one awkward paragraph like this: "Absolute URL. URL that this download started from (before any redirects)." You should rewrite it to either a single sentence or a pair of sentences. E.g. something like: "The absolute URL that this download started from, before any redirects." or "Absolute URL. This will be the URL that this download started from, before any redirects." https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:142: // Actual download URL (final url in chain after all redirects). same nit here https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:295: // Actual download URL (final url in chain after all redirects). same nit here https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. It would be useful to mention here how finalUrl can change, so that it shows up in the documentation for developers. Not having looked at your implementation yet, as a developer consuming the API I'd hope that the only possibility would be that finalUrl would start out empty (so would not be present in any DownloadDelta's) until the download has reached the final url and we are guaranteed no more redirects are possible, and then finalUrl would be non-empty in a DownloadDelta and we're guaranteed it will never change again.
On 2016/06/16 22:11:41, Antony Sargent wrote: > https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... > chrome/common/extensions/api/downloads.idl:363: // The change in > <code>finalUrl</code>, if any. > It would be useful to mention here how finalUrl can change, so that it shows up > in the documentation for developers. Not having looked at your implementation > yet, as a developer consuming the API I'd hope that the only possibility would > be that finalUrl would start out empty (so would not be present in any > DownloadDelta's) until the download has reached the final url and we are > guaranteed no more redirects are possible, and then finalUrl would be non-empty > in a DownloadDelta and we're guaranteed it will never change again. An interesting observation. So far my limited testing shows that this field is never present in delta and I can't make it to appear by any user action. url and finalUrl are only present in download created event. Maybe there is some corner case that I am not aware of, benjhayden@ can you share your knowledge on this subject? To be honest I just added finalUrl to delta to keep consistency with url that way already present there. I've updated other parts of idl documentation, please take another look.
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. On 2016/06/16 at 22:11:41, Antony Sargent wrote: > It would be useful to mention here how finalUrl can change, so that it shows up in the documentation for developers. Not having looked at your implementation yet, as a developer consuming the API I'd hope that the only possibility would be that finalUrl would start out empty (so would not be present in any DownloadDelta's) until the download has reached the final url and we are guaranteed no more redirects are possible, and then finalUrl would be non-empty in a DownloadDelta and we're guaranteed it will never change again. Neither url nor finalUrl can change for a download after it's created. Same goes for mime, and startTime. https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... chrome/browser/download/download_query.cc:226: if (item.GetBrowserContext()) { Download items are indirectly owned by the BrowserContext. item.GetBrowserContext() must always be true for any valid download item. Then again, url_formatter doesn't depend on any specific BrowserContext(). https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... chrome/browser/download/download_query.cc:228: url_formatted = url_formatter::FormatUrl(item.GetURL()); The biggest thing we are doing here is removing unnecessary unescaping. Could you add a comment including an example so that it's easier for readers of this code to understand why this is necessary? https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:806: std::unique_ptr<net::test_server::HttpResponse> RedirectRequestHandler( There's a canned redirect handler that you can use with the default EmbeddedTestServer in browser_tests. See https://cs.chromium.org/chromium/src/net/test/embedded_test_server/default_ha...
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:138: // URL that this download started from (before any redirects). On 2016/06/16 22:11:41, Antony Sargent wrote: > nit: in the autogenerated documentation at > https://developer.chrome.com/extensions/downloads#type-DownloadItem, this will > read as one awkward paragraph like this: > > "Absolute URL. URL that this download started from (before any redirects)." > > You should rewrite it to either a single sentence or a pair of sentences. E.g. > something like: > > > "The absolute URL that this download started from, before any redirects." > > or > > "Absolute URL. This will be the URL that this download started from, before any > redirects." > Done. https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:142: // Actual download URL (final url in chain after all redirects). On 2016/06/16 22:11:40, Antony Sargent wrote: > same nit here Done. https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:295: // Actual download URL (final url in chain after all redirects). On 2016/06/16 22:11:41, Antony Sargent wrote: > same nit here Done. https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. On 2016/06/22 19:05:13, asanka wrote: > On 2016/06/16 at 22:11:41, Antony Sargent wrote: > > It would be useful to mention here how finalUrl can change, so that it shows > up in the documentation for developers. Not having looked at your implementation > yet, as a developer consuming the API I'd hope that the only possibility would > be that finalUrl would start out empty (so would not be present in any > DownloadDelta's) until the download has reached the final url and we are > guaranteed no more redirects are possible, and then finalUrl would be non-empty > in a DownloadDelta and we're guaranteed it will never change again. > > > Neither url nor finalUrl can change for a download after it's created. Same goes > for mime, and startTime. So in what way should be this handled? 1. Just remove finalUrl and leave this part of api a bit inconsistent. 2. Remove all the data that cannot appear here pointed by asanka. This will in theory change the api that should be stable but in practice since the data is optional and never was present it should cause no harm to remove it. 3. Just leave it as it is now for api consistency even though it won't be present. Please advise. https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... chrome/browser/download/download_query.cc:226: if (item.GetBrowserContext()) { On 2016/06/22 19:05:14, asanka wrote: > Download items are indirectly owned by the BrowserContext. > item.GetBrowserContext() must always be true for any valid download item. > > Then again, url_formatter doesn't depend on any specific BrowserContext(). This is leftover from times when formatter required accept languages to format url which were fetched from profile. https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... chrome/browser/download/download_query.cc:226: if (item.GetBrowserContext()) { On 2016/06/22 19:05:14, asanka wrote: > Download items are indirectly owned by the BrowserContext. > item.GetBrowserContext() must always be true for any valid download item. > > Then again, url_formatter doesn't depend on any specific BrowserContext(). Done. https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/download... chrome/browser/download/download_query.cc:228: url_formatted = url_formatter::FormatUrl(item.GetURL()); On 2016/06/22 19:05:14, asanka wrote: > The biggest thing we are doing here is removing unnecessary unescaping. Could > you add a comment including an example so that it's easier for readers of this > code to understand why this is necessary? Done. https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1706193002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:806: std::unique_ptr<net::test_server::HttpResponse> RedirectRequestHandler( On 2016/06/22 19:05:14, asanka wrote: > There's a canned redirect handler that you can use with the default > EmbeddedTestServer in browser_tests. > > See > https://cs.chromium.org/chromium/src/net/test/embedded_test_server/default_ha... Done.
https://codereview.chromium.org/1706193002/diff/80001/chrome/browser/download... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1706193002/diff/80001/chrome/browser/download... chrome/browser/download/download_query.cc:225: // way. This will unescape spaces and trim all extra data (like username and Note that UnescapeRule::SPACE also implies UnescapeRule::NORMAL. It's not just spaces that are going to be affected.
https://codereview.chromium.org/1706193002/diff/80001/chrome/browser/download... File chrome/browser/download/download_query.cc (right): https://codereview.chromium.org/1706193002/diff/80001/chrome/browser/download... chrome/browser/download/download_query.cc:225: // way. This will unescape spaces and trim all extra data (like username and On 2016/06/23 15:48:12, asanka wrote: > Note that UnescapeRule::SPACE also implies UnescapeRule::NORMAL. It's not just > spaces that are going to be affected. Reworded the comment.
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. On 2016/06/23 10:33:21, mharanczyk wrote: > On 2016/06/22 19:05:13, asanka wrote: > > On 2016/06/16 at 22:11:41, Antony Sargent wrote: > > > It would be useful to mention here how finalUrl can change, so that it shows > > up in the documentation for developers. Not having looked at your > implementation > > yet, as a developer consuming the API I'd hope that the only possibility would > > be that finalUrl would start out empty (so would not be present in any > > DownloadDelta's) until the download has reached the final url and we are > > guaranteed no more redirects are possible, and then finalUrl would be > non-empty > > in a DownloadDelta and we're guaranteed it will never change again. > > > > > > Neither url nor finalUrl can change for a download after it's created. Same > goes > > for mime, and startTime. > > So in what way should be this handled? > 1. Just remove finalUrl and leave this part of api a bit inconsistent. > 2. Remove all the data that cannot appear here pointed by asanka. This will in > theory change the api that should be stable but in practice since the data is > optional and never was present it should cause no harm to remove it. > 3. Just leave it as it is now for api consistency even though it won't be > present. > Please advise. I think it's fine to remove any optional elements from this dictionary if they have never, and can never, be present.
On 2016/06/23 16:35:52, Antony Sargent wrote: > https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... > File chrome/common/extensions/api/downloads.idl (right): > > https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... > chrome/common/extensions/api/downloads.idl:363: // The change in > <code>finalUrl</code>, if any. > On 2016/06/23 10:33:21, mharanczyk wrote: > > On 2016/06/22 19:05:13, asanka wrote: > > > On 2016/06/16 at 22:11:41, Antony Sargent wrote: > > > > It would be useful to mention here how finalUrl can change, so that it > shows > > > up in the documentation for developers. Not having looked at your > > implementation > > > yet, as a developer consuming the API I'd hope that the only possibility > would > > > be that finalUrl would start out empty (so would not be present in any > > > DownloadDelta's) until the download has reached the final url and we are > > > guaranteed no more redirects are possible, and then finalUrl would be > > non-empty > > > in a DownloadDelta and we're guaranteed it will never change again. > > > > > > > > > Neither url nor finalUrl can change for a download after it's created. Same > > goes > > > for mime, and startTime. > > > > So in what way should be this handled? > > 1. Just remove finalUrl and leave this part of api a bit inconsistent. > > 2. Remove all the data that cannot appear here pointed by asanka. This will in > > theory change the api that should be stable but in practice since the data is > > optional and never was present it should cause no harm to remove it. > > 3. Just leave it as it is now for api consistency even though it won't be > > present. > > Please advise. > > I think it's fine to remove any optional elements from this dictionary if they > have never, and can never, be present. Are you ok to leave it in this CL as is and remove it in another CL? I don't want to mix those changes up if possible, there is small chance that those fields might be needed after all, so would be good if only removal is reverted if need arises.
> Are you ok to leave it in this CL as is and remove it in another CL? I don't > want to mix those changes up if possible, there is small chance that those > fields might be needed after all, so would be good if only removal is reverted > if need arises. Sure, that seems reasonable.
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. On 2016/06/23 16:35:52, Antony Sargent wrote: > On 2016/06/23 10:33:21, mharanczyk wrote: > > On 2016/06/22 19:05:13, asanka wrote: > > > On 2016/06/16 at 22:11:41, Antony Sargent wrote: > > > > It would be useful to mention here how finalUrl can change, so that it > shows > > > up in the documentation for developers. Not having looked at your > > implementation > > > yet, as a developer consuming the API I'd hope that the only possibility > would > > > be that finalUrl would start out empty (so would not be present in any > > > DownloadDelta's) until the download has reached the final url and we are > > > guaranteed no more redirects are possible, and then finalUrl would be > > non-empty > > > in a DownloadDelta and we're guaranteed it will never change again. > > > > > > > > > Neither url nor finalUrl can change for a download after it's created. Same > > goes > > > for mime, and startTime. > > > > So in what way should be this handled? > > 1. Just remove finalUrl and leave this part of api a bit inconsistent. > > 2. Remove all the data that cannot appear here pointed by asanka. This will in > > theory change the api that should be stable but in practice since the data is > > optional and never was present it should cause no harm to remove it. > > 3. Just leave it as it is now for api consistency even though it won't be > > present. > > Please advise. > > I think it's fine to remove any optional elements from this dictionary if they > have never, and can never, be present. As it turn out those delta fields are actually set up in very specific cases. The onChanged event with all the data from created event can be sent to extension for downloads already existing before onChanged event listener registration or when dangerous download will transition from temporary to not temporary. Otherwise those properties are reported in onCreated event and will never be present in onChanged afaict. I've added additional explanation to documentation stating those situations and mentioning that above download properties may not change after it starts. benjhayden@, asargent@, please take another look.
https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/40001/chrome/common/extension... chrome/common/extensions/api/downloads.idl:363: // The change in <code>finalUrl</code>, if any. On 2016/06/28 at 13:15:32, mharanczyk wrote: > On 2016/06/23 16:35:52, Antony Sargent wrote: > > On 2016/06/23 10:33:21, mharanczyk wrote: > > > On 2016/06/22 19:05:13, asanka wrote: > > > > On 2016/06/16 at 22:11:41, Antony Sargent wrote: > > > > > It would be useful to mention here how finalUrl can change, so that it > > shows > > > > up in the documentation for developers. Not having looked at your > > > implementation > > > > yet, as a developer consuming the API I'd hope that the only possibility > > would > > > > be that finalUrl would start out empty (so would not be present in any > > > > DownloadDelta's) until the download has reached the final url and we are > > > > guaranteed no more redirects are possible, and then finalUrl would be > > > non-empty > > > > in a DownloadDelta and we're guaranteed it will never change again. > > > > > > > > > > > > Neither url nor finalUrl can change for a download after it's created. Same > > > goes > > > > for mime, and startTime. > > > > > > So in what way should be this handled? > > > 1. Just remove finalUrl and leave this part of api a bit inconsistent. > > > 2. Remove all the data that cannot appear here pointed by asanka. This will in > > > theory change the api that should be stable but in practice since the data is > > > optional and never was present it should cause no harm to remove it. > > > 3. Just leave it as it is now for api consistency even though it won't be > > > present. > > > Please advise. > > > > I think it's fine to remove any optional elements from this dictionary if they > > have never, and can never, be present. > > As it turn out those delta fields are actually set up in very specific cases. The onChanged event with all the data from created event can be sent to extension for downloads already existing before onChanged event listener registration or when dangerous download will transition from temporary to not temporary. > Otherwise those properties are reported in onCreated event and will never be present in onChanged afaict. FTR, both of those behaviors are incorrect. Reporting all the fields during onChanged for some downloads isn't useful and masks the material change that occurred in the download. Also, I'd argue that the url, finalUrl and mime fields are more important for evaluating a dangerous download than a safe one. I suggest we address these later. > I've added additional explanation to documentation stating those situations and mentioning that above download properties may not change after it starts. > benjhayden@, asargent@, please take another look.
I am a bit confused, this review is a bit stuck, do you want me to do something more here?
No objections from me other than those pointed out here. https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_query_unittest.cc (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/downloa... chrome/browser/download/download_query_unittest.cc:249: GURL match_url("http://query.com/query"); Should also test with something like http://example.com/qu%65ry to test the unescape logic. https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:2577: " \"finalUrl\": \"%s\"," Are all these test changes necessary? I'm inclined to depend on a minimal set of properties that are necessary to verify the specific behavior being tested. In this case (and in others), checking one of finalUrl or url should be sufficient since the tests are independent of there being any redirects. https://codereview.chromium.org/1706193002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/downloads.idl:551: // current value. Let's not document this behavior; it's a bug and should be fixed at some point (could you file a bug?). Even if the bug were not not be fixed, the behavior changes depending on whether there was some other extension listening for the affected events (i.e. if there was another extension listening, then the new extension that adds a listener will not see any of the additional fields.)
https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_query_unittest.cc (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/downloa... chrome/browser/download/download_query_unittest.cc:249: GURL match_url("http://query.com/query"); On 2016/07/07 19:28:31, asanka wrote: > Should also test with something like http://example.com/qu%65ry to test the > unescape logic. Added unescaping tests for original url and url query filtering. https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:2577: " \"finalUrl\": \"%s\"," On 2016/07/07 19:28:31, asanka wrote: > Are all these test changes necessary? I'm inclined to depend on a minimal set of > properties that are necessary to verify the specific behavior being tested. In > this case (and in others), checking one of finalUrl or url should be sufficient > since the tests are independent of there being any redirects. I've removed most of additions, left finalUrl checks only in basic (to test that they are the same) and redirect test (to test that they are different). https://codereview.chromium.org/1706193002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/1706193002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/downloads.idl:551: // current value. On 2016/07/07 19:28:31, asanka wrote: > Let's not document this behavior; it's a bug and should be fixed at some point > (could you file a bug?). > > Even if the bug were not not be fixed, the behavior changes depending on whether > there was some other extension listening for the affected events (i.e. if there > was another extension listening, then the new extension that adds a listener > will not see any of the additional fields.) Reverted download delta documentation back to original. Created crbug.com/626626
lgtm from me. Thanks for filing the bug and sticking with this :)
asargent@ are you ok with committing this?
On 2016/07/08 14:20:39, mharanczyk wrote: > asargent@ are you ok with committing this? yep, still lgtm
The CQ bit was checked by mharanczyk@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1706193002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose final download URL (actual url after redirects) in the extension API. BUG=620630 ========== to ========== Expose final download URL (actual url after redirects) in the extension API. BUG=620630 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Expose final download URL (actual url after redirects) in the extension API. BUG=620630 ========== to ========== Expose final download URL (actual url after redirects) in the extension API. BUG=620630 Committed: https://crrev.com/235b8cbc6cff62240ea0628fc0d1c5ba4bec38e8 Cr-Commit-Position: refs/heads/master@{#404619} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/235b8cbc6cff62240ea0628fc0d1c5ba4bec38e8 Cr-Commit-Position: refs/heads/master@{#404619} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
