Mark channels (xhr/fetch/.src=) created by scripts handling direct user interaction as urgent-start

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: mayhemer, Assigned: tt)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
mayhemer
: review+
baku
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
mayhemer
: review+
Details
The idea is to e.g. bring an image fancybox to the user ASAP after a click.  The urgent-start classification will tell the network requests scheduler to perform it with the highest priority and also ignoring any parallelism or overall connection limits.
Blocks: CDP
Ideally this bit of meta-data would get properly passed through the nsIChannel to the Request object when we trigger a FetchEvent in a service worker.  That way we could properly pass it through when the SW is doing:

  respondWith(fetch(evt.request))
Priority: -- → P2
As discussion with Shianyow and Tom, this is needed for quantum networking with the expected timeframe by FF56, and Tom is going to help. According to the quick investigation by Tom, this looks doable now and not heavily impacted by the SW redesign. :bkelly, please advise if there's something missing. Thank you.
Assignee: nobody → ttung
I reckon that I should mark the request as urgent-start if a request is created after user interaction event. Besides, the request is not handled by a worker.

For setting flag, I can refer [1] to set urgent-start to channel. Moreover, I need to make sure the flag is passing through SW and redirected request.

Currently, I found EventStateManager::IsHandlingUserInput()[2] may be the function to decide the request is triggered due to user interaction event.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1348053#c7

#include "nsIClassOfService.h"
...
nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
if (cos) {
  cos->AddClassFlags(nsIClassOfService::UrgentStart);
}

[2] http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#3856
(In reply to Tom Tung [:tt] from comment #3)
> I reckon that I should mark the request as urgent-start if a request is
> created after user interaction event. 

Yes.

> Besides, the request is not handled by
> a worker.

I can't say.  You are probably right.  The purpose of urgent-start is to get visual feedback (that something is happening) and optimally also the desired content's first bytes ASAP.  Background work probably doesn't belong to it, if I understand correctly.

> 
> For setting flag, I can refer [1] to set urgent-start to channel. Moreover,
> I need to make sure the flag is passing through SW and redirected request.

Very good point!  I found that redirected channels don't inherit it.  Filed a bug, you don't need to care, it will be fixed.

According SW, it's more a question for :jdm or :bkelly.

> 
> Currently, I found EventStateManager::IsHandlingUserInput()[2] may be the
> function to decide the request is triggered due to user interaction event.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1348053#c7
> 
> #include "nsIClassOfService.h"
> ...
> nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
> if (cos) {
>   cos->AddClassFlags(nsIClassOfService::UrgentStart);
> }
> 
> [2]
> http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.
> cpp#3856

Hmm.. the default for this is 1000ms since the last interaction.  I'm afraid that scripts that may do something soon during a page load, e.g from DOMContentLoaded, could flood the urgent-start queues and limits.  Such network requests should be handled as "normal" parallel loads, IMO.  But if it becomes too complicated to find out something is use interaction triggered, let's try this way.
(In reply to Honza Bambas (:mayhemer) from comment #4)
Thanks for the quick feedback!

> I can't say.  You are probably right.  The purpose of urgent-start is to get
> visual feedback (that something is happening) and optimally also the desired
> content's first bytes ASAP.  Background work probably doesn't belong to it,
> if I understand correctly.

I'll double check it before starting to implement.

> According SW, it's more a question for :jdm or :bkelly.

Thanks, I'll ask them when I have questions for SW.
 
> Hmm.. the default for this is 1000ms since the last interaction.  I'm afraid

It returns false when sUserInputEventDepth gets zero which is decreased by StopHandlingUserInput() as well (after notifying event listener).

> that scripts that may do something soon during a page load, e.g from
> DOMContentLoaded, could flood the urgent-start queues and limits.  Such
> network requests should be handled as "normal" parallel loads, IMO.  But if
> it becomes too complicated to find out something is use interaction
> triggered, let's try this way.

Hmm, I don't think we treat DOMContentLoaded as user input event, but I'm not pretty sure about that. If we don't treat it as user input event, these network requests are handled as normal parallel loads. I'll check it out. 

I have an another question about promise and setTimeout. Should I mark the request inside them as urgent-start if promise or setTimeout are triggered by user interaction event? I have that question because these requests are suspended in original scenario.
(In reply to Tom Tung [:tt] from comment #5)
> that scripts that may do something soon during a page load, e.g from
> DOMContentLoaded, could flood the urgent-start queues and limits.  Such

Oh, I realized you meant one second is too long and it makes other requests become urgent-start. If "sUserInputEventDepth" get zero, we should be able to avoid that.
(In reply to Tom Tung [:tt] from comment #6)
> (In reply to Tom Tung [:tt] from comment #5)
> > that scripts that may do something soon during a page load, e.g from
> > DOMContentLoaded, could flood the urgent-start queues and limits.  Such
> 
> Oh, I realized you meant one second is too long and it makes other requests
> become urgent-start. If "sUserInputEventDepth" get zero, we should be able
> to avoid that.

Yes, exactly.  Then this is the right API to use, thanks!
Hi Honza,

I still have a question about creating a request on another tasks or microtasks (e.g. setTimeout, promise). I know we should mark the request which is triggered by user input event directly. But, what about those request which is triggered indirectly (requests in the tasks created by user input triggering task)?

For example:
```
addEventListener('click', function () {
  fetch("foo1.com"); // Should mark as urgent-start
  setTimeout(_ => fetch("foo2.com"), 0); // Will be execute after fetch("foo3.com"). Not sure if marking this as urgent-start or not
  fetch("foo3.com"); // Should mark as urgent-start
});
```
Should I need to mark the request insides setTimeout/promise as urgentStart since they will be executed later?

If we want to do that, I may need to adding urgent start flag on tasks and their children tasks which are trigger initially by user input event.
Flags: needinfo?(honzab.moz)
Current progress:

- Part 1: Mark the flag on fetch and xhr first since they are easier to mark the urgentStart flag compare to .src.
  - ToDo items: Should I explicitly check if the caller is worker? When the request is handled by worker, they should not found the any user input event in event state manager. But, I'm not sure about that, so I will check it out.

- Part 2: test for fetch and xhr
  - ToDo items: 1. not sure how to check the channel's flag while sending a fetch or xhr. 
                2. test for worker

Since we have many elements which have src attribute. I will do it later. I'll have Part 3 for src and Part 4 for the test of src. 
I put the results for .src I found so far:

grep -r "attribute DOMString src;" dom/webidl
dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;

grep -rn "GetAttr" dom | grep "nsGkAtoms::src"
dom/base/nsFrameLoader.cpp:818:                  mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc,
dom/base/nsFrameLoader.cpp:2552:    mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, aURI);
dom/base/nsObjectLoadingContent.cpp:1716:    thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, uriStr);
dom/html/HTMLImageElement.cpp:986:    if (!GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLImageElement.cpp:1228:  if (!aSourceNode->GetAttr(kNameSpaceID_None, nsGkAtoms::srcset, srcset)) {
dom/html/HTMLImageElement.cpp:1252:    if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src) && !src.IsEmpty()) {
dom/html/HTMLInputElement.cpp:4980:      GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri) &&
dom/html/HTMLInputElement.cpp:5190:    if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLMediaElement.cpp:1947:  } else if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLMediaElement.cpp:2243:    if (!child->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLScriptElement.cpp:281:  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLTrackElement.cpp:281:  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/media/TextTrack.cpp:357:    if (!(mTrackElement->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {
dom/xul/templates/nsXULTreeBuilder.cpp:661:        cell->GetAttr(kNameSpaceID_None, nsGkAtoms::src, raw);
Updated the patch for testing fetch & XHR:
- Finished XHR part
  - Create an XHR via XPCOM interface since I can get its channel from this interface. Thus I can check if the urgent start flag is set in cos flag.
  - Move the test to chrome mochi test since accessing xhr.channel needs chrome privilege.
  - Test it in nine situations(123 * abc): 
      send an XHR 
        1. without event triggering 2. with user input event triggering 3. with non-user input event triggering.
        a. in the original task     b. in an another micro-task (promise)  c. in an another task (setTimeout)
Attachment #8860274 - Attachment is obsolete: true
Hi Honza,

I've finished fetch and xhr part. I'll send a review request to baku as well after getting your r+.

In the Part 1, I checked the EventStateManager if there is triggered by user input event when we just set the fetch request and it is about to send xhr request. Then store it as a flag and check its value after the channel is created.

In the Part 2, I wrote a test for verify the behavior.
Note: I'm not sure about whether should I take care about the created micro-task (promise test) and task (setTimeout). If you think we should set the flag as well when they are triggered by user input events, I will file a follow-up bug for it.

I'll update the patch for src and its test when I finish it.
Clear ni? since I put it in the patch (Part2) for review
Flags: needinfo?(honzab.moz)
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review137746

::: dom/fetch/FetchDriver.cpp:260
(Diff revision 3)
>    }
>  #endif
>    chan->SetNotificationCallbacks(this);
>  
> +  nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(chan));
> +  if (cos && mUseUrgentStartForChannel) {

can you explain why you can't use EventStateManager::IsHandlingUserInput() here?

::: dom/xhr/XMLHttpRequestMainThread.cpp:2658
(Diff revision 3)
>    // deadlock where server generation of CSS/JS requires an XHR signal.
>    nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(mChannel));
>    if (cos) {
>      cos->AddClassFlags(nsIClassOfService::Unblocked);
> +
> +    if (mUseUrgentStartForChannel) {

here as well?
Attachment #8860273 - Flags: review?(honzab.moz) → review+
Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review137748

::: dom/base/test/test_urgent_start.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

I prefer to have a comment at the top of a test that describes what the test is testing and what steps it takes so that one doesn't need to study the code to understand it.

::: dom/base/test/test_urgent_start.html:9
(Diff revision 1)
> +  <title>Test for urgentStart(fetch and xhr)</title>
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet"
> +        type="text/css"
> +        href="chrome://mochikit/content/tests/SimpleTest/test.css"?>

some fishy char at the tail
Attachment #8862352 - Flags: review?(honzab.moz) → review+
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review137746

> can you explain why you can't use EventStateManager::IsHandlingUserInput() here?

I don't have a strong reason for it. I'll move EventStateManager::IsHandlingUserInput() to here.

I wanted to check EventStateManager::IsHandlingUserInput() only when we have windows(not a worker). Thus, I added it ineside the check for windows[1] so that I don't need to check it again.

[1] http://searchfox.org/mozilla-central/source/dom/fetch/Fetch.cpp#342-379

> here as well?

Same as above. I'll move it to here. Thanks!
Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review137748

> I prefer to have a comment at the top of a test that describes what the test is testing and what steps it takes so that one doesn't need to study the code to understand it.

Sure!

> some fishy char at the tail

Oops, I somehow miss that... I'll correct it. Thanks!
Addressed comment and sent r? to :baku.

Hi Honza,

If you see there are still somethings wrong, please feel free to point it out. Thanks!
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

::: dom/xhr/XMLHttpRequestMainThread.cpp:2661
(Diff revision 4)
> +
> +    // Mark channel as urgent-start if the XHR is triggered by user input
> +    // events. Note: we should not set the channel for worker.
> +    nsCOMPtr<nsIDocument> responsibleDoc = GetDocumentIfCurrent();
> +    if (responsibleDoc &&
> +        responsibleDoc->NodePrincipal() == mPrincipal &&

Random comment, why do we need the principal check?
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

> Random comment, why do we need the principal check?

Hi Olli,

Thanks for the feedback! 

I wanted to check whehter it is initiated by user input events. Therefore, I added the document and principal check to prevent it from being called by worker. Thus, I copied the check for worker [1].

[1] http://searchfox.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2473-2477
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138766

::: dom/xhr/XMLHttpRequestMainThread.cpp:2661
(Diff revision 4)
> +
> +    // Mark channel as urgent-start if the XHR is triggered by user input
> +    // events. Note: we should not set the channel for worker.
> +    nsCOMPtr<nsIDocument> responsibleDoc = GetDocumentIfCurrent();
> +    if (responsibleDoc &&
> +        responsibleDoc->NodePrincipal() == mPrincipal &&

I don't think EventStateManager::IsHandlingUserInput() returns true if there are workers involved. if this is used by workers, this method has been called by a runnable.
Attachment #8860273 - Flags: review?(amarchesini) → review+
Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review138768
Attachment #8862352 - Flags: review?(amarchesini) → review+
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138766

> I don't think EventStateManager::IsHandlingUserInput() returns true if there are workers involved. if this is used by workers, this method has been called by a runnable.

I see, so I'll remove the extra check for the worker (document check in FetchDriver, document and principal checks in XHR). Thanks!
Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

> Hi Olli,
> 
> Thanks for the feedback! 
> 
> I wanted to check whehter it is initiated by user input events. Therefore, I added the document and principal check to prevent it from being called by worker. Thus, I copied the check for worker [1].
> 
> [1] http://searchfox.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2473-2477

I'll remvoe it based on Andrea's comment.
Hi Honza,

I greped the "attribute DOMString src;" from dom/webidl and I got:
dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;

I've finished marking urgent-start for HTMLEmbedElement, HTMLFrameElement, HTMLIFrameElement, HTMLImageElement, HTMLInputElement, HTMLMediaElement and HTMLScriptElement. I'll send them to review once the try goes green.

However, I think marking urgent-start for src attributes of HTMLScriptElement, HTMLTrackElement and SpeechGrammar are not necessary. 

The reason for script element is that it has its own mechanism to preload the script. As for the track element (webvtt), it usually open channel for loading txt (e.g. subtitle). For the SpeechGrammar, I think it's not related to this bug.

Thus, I won't mark urgent-start for them.
Please correct me if you have different thoughts for this.
Flags: needinfo?(honzab.moz)
(In reply to Tom Tung [:tt] from comment #36)
> Hi Honza,
> 
> I greped the "attribute DOMString src;" from dom/webidl and I got:
> dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
> dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
> dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;
> 
> I've finished marking urgent-start for HTMLEmbedElement, HTMLFrameElement,
> HTMLIFrameElement, HTMLImageElement, HTMLInputElement, HTMLMediaElement and
> HTMLScriptElement. I'll send them to review once the try goes green.
> 
> However, I think marking urgent-start for src attributes of
> HTMLScriptElement, HTMLTrackElement and SpeechGrammar are not necessary. 

I agree.  Actually, this should only be implemented for visual impacting elements, such as img, video, audio, embed.  iframe, frame and script and definitely track are not necessary IMO.

Is SpeechGrammar something people expect to be loaded and played ASAP after a user interaction?  I don't know what SpeechGrammar is tho :)

> 
> The reason for script element is that it has its own mechanism to preload
> the script. As for the track element (webvtt), it usually open channel for
> loading txt (e.g. subtitle). For the SpeechGrammar, I think it's not related
> to this bug.
> 
> Thus, I won't mark urgent-start for them.
> Please correct me if you have different thoughts for this.

I think your decisions are OK!  This doesn't need to be perfect at the first stage.

Thanks!
Flags: needinfo?(honzab.moz)
Comment on attachment 8865301 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

https://reviewboard.mozilla.org/r/136940/#review140022
Attachment #8865301 - Flags: review?(amarchesini) → review+
Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140024

I prefer bz to review this patch. Or any other docShell peer.
Attachment #8865300 - Flags: review?(amarchesini)
Comment on attachment 8865299 [details]
Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

https://reviewboard.mozilla.org/r/136936/#review140026
Attachment #8865299 - Flags: review?(amarchesini) → review+
Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140092

I can only review the bit that adds the class on the channel.  I'm not an imagelib nor a dom peer, you need to find someone else to this review fully.


r+ for only the imgLoader.cpp parts

::: dom/base/nsImageLoadingContent.cpp:661
(Diff revision 1)
>    // We keep this flag around along with the old URI even for failed requests
>    // without a live request object
>    ImageLoadType loadType = \
>      (mCurrentRequestFlags & REQUEST_IS_IMAGESET) ? eImageLoadType_Imageset
>                                                   : eImageLoadType_Normal;
> +

?

::: image/imgLoader.cpp:2284
(Diff revision 1)
>      MOZ_LOG(gImgLog, LogLevel::Debug,
>             ("[this=%p] imgLoader::LoadImage -- Created new imgRequest"
>              " [request=%p]\n", this, request.get()));
>  
> +    nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(newChannel));
> +    if (cos && aUseUrgentStartForChannel) {

same question as in another patch:

why not use EventStateManager::IsHandlingUserInput() just here?  

I don't say the approach you use is wrong, it just needs some explanation why it has to be captured so much above and carried down here via a member and number of arguments.  it's a lot of code changes and when we add a new place where src is handled, it will need an update to set the urgent start thing.
Attachment #8865298 - Flags: review?(honzab.moz) → review+
Comment on attachment 8865299 [details]
Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

https://reviewboard.mozilla.org/r/136936/#review140096

I can't review this.  You need someone from the dom or media team to do it.
Attachment #8865299 - Flags: review?(honzab.moz)
Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140098

bug 1348044 handles this.  and please see https://bugzilla.mozilla.org/show_bug.cgi?id=1348044#c4.  we don't want this for iframes/frames.  I *think* that for nsDocShell::OnLinkClickSync the [isTopLevelDoc && mIsActive || (mLoadType & (LOAD_CMD_NORMAL | LOAD_CMD_HISTORY))] cond will eval to true.

IMO, this patch is a WONTFIX.
Attachment #8865300 - Flags: review?(honzab.moz) → review-
Comment on attachment 8865301 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

https://reviewboard.mozilla.org/r/136940/#review140106

You need a dom peer to r this.
Attachment #8865301 - Flags: review?(honzab.moz)
Comment on attachment 8865302 [details]
Bug 1348050 - Part 6: Expand the original test to verify src attribute.

https://reviewboard.mozilla.org/r/136942/#review140112
Attachment #8865302 - Flags: review?(honzab.moz) → review+
Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140092

Thanks!

> ?

Sorry, I somehow added this empty line and forgot to remove it...

> same question as in another patch:
> 
> why not use EventStateManager::IsHandlingUserInput() just here?  
> 
> I don't say the approach you use is wrong, it just needs some explanation why it has to be captured so much above and carried down here via a member and number of arguments.  it's a lot of code changes and when we add a new place where src is handled, it will need an update to set the urgent start thing.

Rather than checking the IsHandlingUserInput just after creating a channel, I checked IsHandlingUserInput and store it when we store the src URI. Then, I passed the flag to the function where we create the channel.

The reason it that when we load a image in responsive mode[1], we will queue the loading task and run it in stable state[2]. I was afriad the task might be queue'd more than one second which will make the user input check invaild [3]. 

Thus, I chose to check the flag when we set the src attribute or something else that may trigger a image load to prevent it from running more than one second. 

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#420
[2] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#916-920
[3] http://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#309
Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140098

I see. I'll remove this patch. Thanks!
Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140024

I'll remove this patch due to Honza's comment.
Attachment #8865300 - Attachment is obsolete: true
Comment on attachment 8865302 [details]
Bug 1348050 - Part 6: Expand the original test to verify src attribute.

https://reviewboard.mozilla.org/r/136942/#review140498
Attachment #8865302 - Flags: review?(amarchesini) → review+
Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140502
Attachment #8865298 - Flags: review?(amarchesini) → review+
Attachment #8865297 - Attachment is obsolete: true
(In reply to Andrea Marchesini [:baku] from comment #58)
(In reply to Honza Bambas (:mayhemer) from comment #53)

Thanks for reviews!!
(In reply to Tom Tung [:tt] from comment #54)
> The reason it that when we load a image in responsive mode[1], we will queue
> the loading task and run it in stable state[2]. I was afriad the task might
> be queue'd more than one second which will make the user input check invaild
> [3]. 
> 

Aha.  But if the task is queued for so long time it probably doesn't make any sense to try to urge the network request then.  Anyway, the patch's been written, let's leave it. 

Hmm..  I absolutely don't know this code, but should we give src assignment coming from a user input (for images) some higher priority?  definitely for a different bug!

Thanks.
(In reply to Honza Bambas (:mayhemer) from comment #66)
> Aha.  But if the task is queued for so long time it probably doesn't make
> any sense to try to urge the network request then.  Anyway, the patch's been
> written, let's leave it. 
> 
> Hmm..  I absolutely don't know this code, but should we give src assignment
> coming from a user input (for images) some higher priority?  definitely for
> a different bug!
> 
> Thanks.

Sorry, I find I forgot to publish the another comment in review board. Still try to get used into it...

There is an stronger reason for it. Since it will create another runnable(task) and run it in stable state, event has gone when we check it. Thus, we need to check it before we create the runnable and pass it when we start to run that runnable. 

I meant that if we only check event for image load just after creating channel, it will always get false in responsive mode.
(In reply to Honza Bambas (:mayhemer) from comment #66)
> Hmm..  I absolutely don't know this code, but should we give src assignment
> coming from a user input (for images) some higher priority?  definitely for
> a different bug!

Hmm, I thought the ugrent-start is the highest one. If we have a higher one, I reckon we could do that, but I don't have a strong opinion for it.
(In reply to Tom Tung [:tt] from comment #68)
> (In reply to Honza Bambas (:mayhemer) from comment #66)
> > Hmm..  I absolutely don't know this code, but should we give src assignment
> > coming from a user input (for images) some higher priority?  definitely for
> > a different bug!
> 
> Hmm, I thought the ugrent-start is the highest one. If we have a higher one,
> I reckon we could do that, but I don't have a strong opinion for it.

I might express myself in a confusing way.  'urgent-start' is telling the network scheduler to put the request (represented by the channel) on the wire ASAP, no need for any more fiddle with priorities.  

What I suggest above is to change also the code in dom/image handling.  There seems to be a delay (that can be more than a second?) introduced as part of the "responsive mode" handling before we even create the network channel.
(In reply to Honza Bambas (:mayhemer) from comment #69)
> I might express myself in a confusing way.  'urgent-start' is telling the
> network scheduler to put the request (represented by the channel) on the
> wire ASAP, no need for any more fiddle with priorities.  

Understand!

> What I suggest above is to change also the code in dom/image handling. 
> There seems to be a delay (that can be more than a second?) introduced as
> part of the "responsive mode" handling before we even create the network
> channel.

I think I might mislead you... Sorry for that :(

In general, it should not delay so long especially Q-DOM marking the foreground task as a higher priority. 

I think the reason why we put this into another microtask/task to awit another metastable/stable state is that we don't want to occpuy main thread too much time and we can do some other microtask/task between them. Besides, it is written in the spec, so the other broswer should handle them similar to this as well.

Anyway, I just tried to implement a way that can handle all the situations which I can imagine even it rarely happens. It doesn't mean that we usually have a delay more than one second for loading an image.
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e11ea0ec07a4
Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/60b9bc8fd85a
Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/3011f497829a
Part 3: Mark channel as urgent-start for loading image. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/98c06ef34ca8
Part 4: Mark channel as urgent-start for loading media. r=baku
https://hg.mozilla.org/integration/autoland/rev/d2d736b78d1e
Part 5: Mark channel as urgent-start for embed element. r=baku
https://hg.mozilla.org/integration/autoland/rev/05ff3de940c7
Part 6: Expand the original test to verify src attribute. r=baku,mayhemer
Thank you Tom!
(In reply to Shian-Yow Wu [:swu] from comment #74)
> Thank you Tom!

np! Thanks for review and feedback, Honza, Andrea! :)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.