1360603 - Mark Save as... channels used for download as throttleable

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Honza Bambas (:mayhemer)]

(In reply to :Paolo Amadini from comment #12)
> It seems to me that downloads started via nsIWebBrowserPersist are still not
> throttled. These may include downloads started by "Save Page As" or "Save
> Link As".

Comment 1

[Honza Bambas (:mayhemer)]

Looks like we want to add the Throttleable class somewhere at https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/dom/webbrowserpersist/nsWebBrowserPersist.cpp#1497

Comment 2

[Honza Bambas (:mayhemer)]

Fairly easy and is a win.

Comment 3

[Amy Chung [:Amy]]

Attachment: implementation -- Marked the save channel as throttleable.

Hi Nick,
I had set the class flags as throttleable when using save channel.
Would you help me to review my patch?

Thanks!

Comment 4

[u408661]

Comment on attachment 8904930 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Review of attachment 8904930 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally reasonable to me, but I don't know nsWebBrowserPersist at all. You need to have a dom peer review this.

::: dom/webbrowserpersist/nsWebBrowserPersist.cpp
@@ +1526,5 @@
>      // Add the output transport to the output map with the channel as the key
>      nsCOMPtr<nsISupports> keyPtr = do_QueryInterface(aChannel);
>      mOutputMap.Put(keyPtr, new OutputData(aFile, mURI, aCalcFileExt));
>  
> +    // Remove throttleable when save channel which finishs the saving work.

"Remove throttleable flag when the save channel finishes"

Comment 5

[Amy Chung [:Amy]]

Attachment: implementation -- Marked the save channel as throttleable.

Hi Nathan,
I had set the class flags as throttleable when using save channel.
Would you help me to review my patch?

Thanks!

Comment 6

[Amy Chung [:Amy]]

Comment on attachment 8906934 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Hi Ehsan,
I had set the class flags as throttleable when using save channel.
Would you help me to review my patch?

Thanks!

Comment 7

[Amy Chung [:Amy]]

Comment on attachment 8906934 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Hi Bobby,
I had set the class flags as throttleable when using save channel.
Would you help me to review my patch?

Thanks!

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8906934 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

I don't know this code and don't have cycles to dig into it right now - sorry about that.

Comment 9

[Amy Chung [:Amy]]

Comment on attachment 8906934 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Hi Baku,
I had set the class flags as throttleable when using save channel.
Would you help me to review my patch?

Thanks!

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8906934 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Review of attachment 8906934 [details] [diff] [review]:
-----------------------------------------------------------------

mayhemer do you have time to take a look at this ClearClassFlags()?

::: dom/webbrowserpersist/nsWebBrowserPersist.cpp
@@ +1498,5 @@
>          return StartUpload(bufferedInputStream, aFile, contentType);
>      }
>  
> +    // Mark save channel as throttleable.
> +    nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(aChannel));

if (cos) { ...

@@ +1527,5 @@
>      nsCOMPtr<nsISupports> keyPtr = do_QueryInterface(aChannel);
>      mOutputMap.Put(keyPtr, new OutputData(aFile, mURI, aCalcFileExt));
>  
> +    // Remove throttleable flag when the save channel finishes.
> +    cos->ClearClassFlags(nsIClassOfService::Throttleable);

Why clearing this flag here? The operation is async.

Comment 11

[Amy Chung [:Amy]]

Attachment: implementation -- Marked the save channel as throttleable.

Hi Baku,
I have modified the patch as below:
1. Added a confirmation for verifying cos is null or not.
2. Removed the removing throttleable flag.

Would you help me to review this patch?
Thanks!

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8908467 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

This patch looks fine, thanks!

Comment 13

[Amy Chung [:Amy]]

Attachment: test case -- Confirmed the Throttleable flag on browser_saveImageURL.js

Hi Baku,
I have finished the test case, and I found browser_saveImageURL.js which is the better test case to confirm throttleable flag.
Would you help me to review my patch?

Thanks!

Comment 14

[Honza Bambas (:mayhemer)]

This seems ready to land, please also request 57 approval (beta), thanks!

Comment 15

[Amy Chung [:Amy]]

Attachment: implementation -- Marked the save channel as throttleable.

Comment 16

[Amy Chung [:Amy]]

Attachment: implementation -- Marked the save channel as throttleable.

Comment 17

[Amy Chung [:Amy]]

Attachment: test case -- Confirmed the Throttleable flag on browser_saveImageURL.js

Comment 18

[Amy Chung [:Amy]]

[Try Server]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8158fd2a9e1ec28daa6d7d0ffd2688cd0efb35c0&selectedJob=132860076

Comment 19

[Amy Chung [:Amy]]

Comment on attachment 8911497 [details] [diff] [review]
implementation -- Marked the save channel as throttleable.

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:unknown
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]:unknown
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:Added a throttleable flags in saving channel.
[String changes made/needed]:none

Comment 20

[Amy Chung [:Amy]]

Comment on attachment 8911498 [details] [diff] [review]
test case -- Confirmed the Throttleable flag on browser_saveImageURL.js

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:unknown
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: unknown
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]: Modified the test case for confirming throttleable flag on browser_saveImageURL.js
[String changes made/needed]:none

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d409c2ac7d88
Mark the save channel as throttleable. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7610f4d26d
Confirm the Throttleable flag on browser_saveImageURL.js. r=baku

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d409c2ac7d88
https://hg.mozilla.org/mozilla-central/rev/6d7610f4d26d

Comment 23

[Sylvestre Ledru [:Sylvestre]]

> [Feature/Bug causing the regression]:unknown
> [User impact if declined]:unknown
> [Needs manual test from QE? If yes, steps to reproduce]:unknown

Could you please be a bit more explicit here?
We need these information to make a call if it is worth taking the risk to uplift that to beta.
Thanks

Comment 24

[Amy Chung [:Amy]]

Hi,
(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> > [Feature/Bug causing the regression]:none
This bug is similar to Bug 1348062, the patches of bug 1348062 seems not to occur the regression bugs.
> > [User impact if declined]:none
> > [Needs manual test from QE? If yes, steps to reproduce]:none
The behavior of this patch is set throttleable flags when user uses "save as.." function, can't use manual test to verify the effect.
> Could you please be a bit more explicit here?
> We need these information to make a call if it is worth taking the risk to
> uplift that to beta.
> Thanks

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911498 [details] [diff] [review]
test case -- Confirmed the Throttleable flag on browser_saveImageURL.js

OK, thanks. Let's take it then!
Should be in 57b4 (gtb tomorrow Thursday)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc0f6c992a6c
https://hg.mozilla.org/releases/mozilla-beta/rev/70517fee99a3