(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".

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

Fairly easy and is a win.

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

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"

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

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 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 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 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 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.

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 on attachment 8908467 [details] [diff] [review] implementation -- Marked the save channel as throttleable. This patch looks fine, thanks!

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!

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

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

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 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

Pushed by ryanvm@gmail.com: 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

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

> [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

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 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)

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