Closed Bug 278531 Opened 20 years ago Closed 19 years ago

generic request prioritization (loadgroup prioritization)

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta1

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

Attachments

(2 files, 1 obsolete file)

v2 patch
32.48 KB, patch
Biesinger
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
nsISupportsPriority.idl
2.97 KB, text/plain
Details 
generic request prioritization (loadgroup prioritization)

this bug is a spin-off from bug 142255.

to make the HTTP channel prioritization easier to use, we should allow a load
priority to be assigned to loadgroups and have that affect all requests
contained in the loadgroup.  this probably means implementing a common interface
like:

  interface nsIPriorityRequest : nsISupports {
    attribute short priority;
  };

then loadgroups, HTTP channels, and image requests would support this interface.

from bug 142255 comment #22:
  "loadgroup::addrequest would do something like:
     aRequest.priority += mPriority;
   instead of =."

and it would probably make sense for loadgroup::removeRequest to undo that (in
case a request is being moved from one loadgroup to another).

instead of introducing nsIPriorityRequest, we could also make use a named
properties interface such as the one I proposed in bug 270224.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
Attached patch v1 patch (obsolete) — — Splinter Review 
Boris and I came up with nsIScheduled as the interface of choice for this.  It
seemed to us that there is no reason to tie the inteface to network requests,
as it could very well be used to express scheduling prioritization of other
sorts of objects, such as threads or processes.

This patch makes nsHttpChannel, imgRequestProxy, nsLoadGroup, and nsDocLoader
all implement nsIScheduled.  I also make NewImageChannel set a default priority
of LOW for images.

It was really neat to visit www.gamespot.com, click to open a new tab on some
content while the page was still loading, and see the new page (the HTML at
least) load before the old one finished loading :)
Attachment #171611 - Flags: superreview?(bzbarsky)
Attachment #171611 - Flags: review?(cbiesinger)
NOTE: the point of implementing nsIScheduled on nsDocLoader is to make it easy
to set a load priority on a "docshell" from the tabbrowser code.  The plan is to
make it so that the current tab has a higher priority than background tabs.
Comment on attachment 171611 [details] [diff] [review]
v1 patch

this is really cool. a few comments right now:
- nsIHttpChannelInternal: it would be nice if it got the same IID it had before
the priority adding (uuid(f3764874-ed7e-4873-883c-11d67a4e3638))

 netwerk/base/src/nsLoadGroup.cpp
+    else if (aIID.Equals(NS_GET_IID(nsIScheduled))) {
+	 *aInstancePtr = NS_STATIC_CAST(nsIScheduled*,this);

missing space before |this|


Would it make sense for nsSocketTransport to implement nsIScheduled as well?
For example, would reordering the mPollList/mActiveList in the socket transport
service make earlier connections faster?
yeah, i agree.. i should revert the uuid of that interface.

as for the socket transport service, i didn't think it mattered where a socket
was placed in the poll list.  do you know of any OS documentation that can shed
some light on the matter?
so, there are three other things out there that deal with schedules. bookmark
updates, rss feeds and calendars. if you're making an interface which doesn't
benefit or accurately describes scheduling in any of those places, then we have
a problem.

at a quick glance, nsIScheduled most certainly doesn't fit with the bookmarks
view of scheduling which is more about how often or when to check for something
and less about prioritizing things. perhaps you should consider the way mailnews
exposes message priorities, since that's a much closer fit.
interesting...  perhaps some things are better expressed using a different
interface.  this interface is meant to capture the OS notion of scheduling tasks
(see "man 2 setpriority" or "man 2 nice").  i'm not sure we want to try to fit
all those shoes into the same boot here.
nsIPriority sounds fine to me, i could even use that in the mail case. I don't
understand why you picked Scheduled over Priority, the interface clearly deals
in priorities, as noted it does so generically, but in terms of priorities, not
schedules....
or nsIPrioritized i suppose
nsIScheduled was really just in response to "nsIPrioritizable" which was just
too painful to type ;-)

nsIPrioritized is better than nsIPrioritizable, so I could go with that too I guess.
nsISupportsPriority ;-)
Comment on attachment 171611 [details] [diff] [review]
v1 patch

Other than timeless's comment about Interface name, my only thought is with the
const in the idl


>Index: xpcom/threads/nsIScheduled.idl
>===================================================================
>+  /**
>+   * Typical priority values.
>+   */
>+  const long HIGHEST = -20;
>+  const long HIGH    = -10;
>+  const long NORMAL  =   0;
>+  const long LOW     =  10;
>+  const long LOWEST  =  20;


Perhaps a

+ const long FIRST = -20;
+ const long LAST = 20;
+ const long STEP = 10;
or some such, incase a caller wishes to enumerate the priorities for some
reason, or do range checking.

I'm not too sure how useful it would be, but would save a caller breaking
incase someone tries to do that at some point.

Nice work, I like ;-)
(In reply to comment #4)
> as for the socket transport service, i didn't think it mattered where a socket
> was placed in the poll list.

I have no idea whether it does :-) hm, thinking about it, it seems really
unlikely that it does.




> or some such, incase a caller wishes to enumerate the priorities for some
> reason, or do range checking.

Such a caller has clearly not read the comments on this interface very well. 
Any 32-bit integer value is a valid priority value, a-priori.
To anyone who cares about the name of this interface, please weigh in with your
opinion by tomorrow.  I'll decide based on feedback at that time and then cut a
new patch.  Thanks!
bz, makes sense to me then I guess
Darin, nsIPrioritized or nsISupportsPriority sounds good to me, no clue on if
there is anything better.
nsISupportsPriority sounds rather bad to me... I'd prefer nsIPrioritized, I think.
Comment on attachment 171611 [details] [diff] [review]
v1 patch

netwerk/protocol/http/src/nsHttpChannel.cpp
+    PRInt16 newValue = CLAMP(value, PR_INT16_MIN, PR_INT16_MAX);

is there a reason for this? the interface allows it, obviously, but why
truncate the value? (is it to make the object a bit smaller?)

modules/libpr0n/src/imgLoader.cpp
so... I think this is OK... but I wonder if it should maybe be
nsContentUtils::LoadImage  that does this scheduling? (almost the only
LoadImage caller)
I'm undecided... feel free to leave it here :-)

pavlov should probably OK the imagelib changes, though.

uriloader/base/nsDocLoader.cpp
+NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority)
+{
+  // Set the priority of our loadgroup, and then set the priority
+  // of each child nsDocLoader instance.

hm... that second part can be removed once bug 261091 is fixed

+  if (mLoadGroup) {
+    nsCOMPtr<nsIScheduled> scheduled = do_QueryInterface(mLoadGroup);

do_QI is nullsafe... no need for the if

+    loader = NS_STATIC_CAST(nsDocLoader*, ChildAt(i));
+    if (loader) {
+      loader->SetPriority(aPriority);

are children of a docloader ever null?


So question... when one sets a priority on a docloader, and a new load starts.
shouldn't the set priority be used for that too?

hm... ok, this may do the right thing, as the loadgroup is reused (I think);
but, it won't work for child docloaders. in fact, new children don't get the
stored priority at all. is this worth fixing? Bug 261091 would automatically
fix it.

r=me, with whatever name you want to pick :-)
Attachment #171611 - Flags: review?(cbiesinger) → review+
the interface really should go in xpcom/ds, xpcom/threads, or xpcom/io.  It
might make sense to expose this on object in xpcom as well.

I like the name nsIPriority, myself, as the only attribute of this interface is
named |priority|.
> the interface really should go in xpcom/ds, xpcom/threads, or xpcom/io.  It
> might make sense to expose this on object in xpcom as well.

doug: thanks for the feedback.  right now, i have the interface in
xpcom/threads, which seems to make sense since it is modeled after "classical"
OS thread priorities.
Comment on attachment 171611 [details] [diff] [review]
v1 patch

>Index: xpcom/threads/nsIScheduled.idl

I have no strong opinions on the name, so whatever the final decision on this
was... ;)

>Index: netwerk/protocol/http/public/nsIHttpChannelInternal.idl

>-[scriptable, uuid(ae9dce68-c27c-44f1-b41d-462f5e470945)]
>+[scriptable, uuid(6f225cf9-dfeb-4b9f-8ff5-2a5bf31b73a1)]

Actually, you're restoring the file back to what it was in rev 1.5.  Want to
just restore that iid as well?	That would make the most sense to me.

>Index: modules/libpr0n/src/imgLoader.cpp

I'm not so happy with this part.  Could you ok it with Stuart?	Does this leave
it possible to load images at the desired priority for imagelib users who don't
want LOW?

Doing this in nsContentUtils instead like biesi suggests would make a lot of
sense, if that would work.  It seems to me that it should.

>Index: modules/libpr0n/src/imgRequestProxy.cpp
>+NS_IMETHODIMP imgRequestProxy::GetPriority(PRInt32 *priority)
>+{
>+  NS_ENSURE_STATE(mOwner);
>+  *priority = mOwner->Priority();
>+  return NS_OK;

So... this can leave priority uninitialized (if mOwner is null).

That's probably fine, but the reprioritization code in loadgroup should
probably not call SetPriority if GetPriority failed.

>Index: uriloader/base/nsDocLoader.cpp
>+NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority)
>+{
>+  // Set the priority of our loadgroup, and then set the priority
>+  // of each child nsDocLoader instance.

Should this change kids by the delta, which is what loadgroups do, instead of
just overriding them?

Could you add a comment to the "loadgroups should be in loadgroups" bug about
removing this code when that's fixed?

sr=bzbarsky with all that addressed.
Attachment #171611 - Flags: superreview?(bzbarsky) → superreview+
> I'm not so happy with this part.  Could you ok it with Stuart?
> Does this leave it possible to load images at the desired priority for 
> imagelib users who don't want LOW?
> 
> Doing this in nsContentUtils instead like biesi suggests would make a lot of
> sense, if that would work.  It seems to me that it should.

The problem here is that from the point of view of nsContentUtils the LoadImage
call could end up piggy backing on an existing load.  If the priority of that
load was already decremented by say nsImageFrame (b/c the image happens to be
associated with a frame -- I have another patch for this), then nsContentUtils
would have to be careful not to override that.

However, by having the priority be set by LoadImage itself (or rather
NewImageChannel), the default is cleanly applied to channels created by image
lib.  This still gives consumers the ability to alter the load priority of the
image request once they have a handle to the image request (i.e., the
imgIRequest returned from LoadImage).  So, I figure that it is simpler to just
set a default load priority for all images created by image lib.


> >Index: modules/libpr0n/src/imgRequestProxy.cpp
> >+NS_IMETHODIMP imgRequestProxy::GetPriority(PRInt32 *priority)
> >+{
> >+  NS_ENSURE_STATE(mOwner);
> >+  *priority = mOwner->Priority();
> >+  return NS_OK;
> 
> So... this can leave priority uninitialized (if mOwner is null).

that's fine.  the method is also returning an exception in that case, so
the value of the out param upon return is undefined anyways.


> That's probably fine, but the reprioritization code in loadgroup should
> probably not call SetPriority if GetPriority failed.

good catch.  i'll fix that.


> >Index: uriloader/base/nsDocLoader.cpp
> >+NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority)
> >+{
> >+  // Set the priority of our loadgroup, and then set the priority
> >+  // of each child nsDocLoader instance.
> 
> Should this change kids by the delta, which is what loadgroups do, instead of
> just overriding them?

hmm... sure.  that's a good idea, though i'm not sure that we have any use cases
where it would matter.


> Could you add a comment to the "loadgroups should be in loadgroups" bug about
> removing this code when that's fixed?

yeah, good idea.
> If the priority of that load was already decremented by say nsImageFrame

The only thing imageframe loads are the placeholder images.  The load of the
"main" image is done through nsContentUtils....
bz,

let me clarify.  i added code in nsImageFrame::Init that decrements the priority
of the imgrequest associated with the imageloadingcontent passed into the
imageframe's init method.  it seemed to me that this was the link between
imageloadingcontent and the frame tree.

that seemed to cause those images to be loaded with higher priority.  my
testcase was to create a bunch of <img> elements via document.createElement and
not actually stick them in the document until some user action.  it had the
effect of giving higher priority to images that were in the DOM, which seems
ideal given that people like to prefetch images for menus and such this way.

please let me know if there is a better way to figure out when an image request
has been associated with a layout frame.
over IRC bz made the excellent point that we have to be careful not to decrement
the priority of the same image request more than once just because it happens to
be associated with multiple image frames.
I see what you mean about the interaction of nsContentUtils and nsImageFrame.

Maybe nsContentUtils should only mess with the priority if the priority is 0? 
But I guess with loadgroup prioritization that leads to problems... hmm....
I suppose I could compare the priority of the loadgroup to the image request to
determine if the image request should be given a boost in priority.  That's
making the image frame know a lot about this system, but that's the best
solution I've come up with so far.
Yeah, that may be the way to go...

Note that we do still want to distinguish between LOAD_BACKGROUND loads and
other image loads in nsContentUtils.  So maybe all the logic of this sort should
live in nsContentUtils methods.
Ok, after talking with stuart on IRC, I decided on a new plan:

1) Make imgRequestProxy::SetPriority only affect the priority of the underlying
imgRequest if it is the first imgRequestProxy.

2) Make imgLoader not set any default priority for channels that it creates.

3) Make nsContentUtils decrease priority for any imgIRequest instances created
by a call to imgILoader::LoadImage.

4) Make nsImageFrame increase priority for any imgIRequest associated with a frame.

This isn't perfect, but I think it'd be a good first solution to this problem. 
Eventually we might want to make imgIRequest return some sort of weight based on
the number of imgRequestProxy instances that can be incorporated into the load
priority of the request.  Or, maybe imagelib can manage this internally.

Also, I'm leaning toward nsISupportsPriority because I think it expresses this
interface best.
Sounds good, as long as the resulting "image associated with frame" priority is
lower than the priority for toplevel documents.

By the way, do we want (in the HTTP code) to slowly increase the priorities of
things that have been sitting in the queue for a long time so that they're
guaranteed to load in a finite amount of time even if other things are being
added to the queue at intervals?
One other thing.  The priority that nsContentUtils sets should depend on whether
the LOAD_BACKGROUND flag is set, imo.
> Sounds good, as long as the resulting "image associated with frame" priority 
> is lower than the priority for toplevel documents.

Agreed.


> By the way, do we want (in the HTTP code) to slowly increase the priorities of
> things that have been sitting in the queue for a long time so that they're
> guaranteed to load in a finite amount of time even if other things are being
> added to the queue at intervals?

That sounds like a good idea.  I'll look into that as a follow-up.


> The priority that nsContentUtils sets should depend on whether
> the LOAD_BACKGROUND flag is set, imo.

Yes.  I'll penalize those by maybe an additional 1 or 2 points perhaps.
OK, I forgot something important:

It's wrong for nsContentUtils::LoadImage to try to set the priority after it has
created the image request.  Why?  Well, as I mentioned earlier, but then
promptly forgot, once AsyncOpen has been called, the request will have been
scheduled.  That means that it will go into the queue with NORMAL priority.  Is
this what we want?  No, probably not.  We want LOW priority.  As a result, we
have two problems: new image requests may displace higher priority stuff (once
we start fetching we don't stop just because the thing became low priority), and
we end up eating CPU cycles to reschedule the request.  It just makes sense to
assign the priority _before_ AsyncOpen is called.  So, how do we do that?

Two solutions:

1) Add a priority parameter to LoadImage so that we don't have to give image
requests a LOW priority by default.

2) Make LoadImage give image requests a LOW priority by default.

Adding new parameters to LoadImage is unattractive given the current number of
parameters, so I vote for option 2.  We can implement 1 if 2 becomes a problem.
 NOTE: remember that this is only going to apply to HTTP loads until someone
implements nsISupportsPriority in some other protocol handler (which seems
unlikely).
BTW, callers of imgILoader::LoadImage can always increase the priority of an
image request without fear of the "displacement problem" that applies when
trying to go the other way.  So, again option 1 :)
So wait.  We call LoadImage().  Say LoadImage() doesn't set the priority itself.
 There are two possibilities:

1) There is nothing in the HTTP request queue; the request gets dispatched.
2) There is something in the HTTP request queue; the request gets queued.

Now we change the priority.  You're right that it won't affect case #1, but in
case #2 it will resort accordingly, no?

Note that if LoadImage sets the priority itself, we'll still get immediate
dispatch in case #1 (since there is nothing in the queue).

So the only drawback of setting priority outside LoadImage is the extra time
spent resorting the HTTP queue...
you forget that the queue is processed on a background thread:  suppose we have
a couple items in the queue that have priority 5, and then imagelib inserts an
image with priority 0.  then we give that image priority 10 immediately
following its insertion into the queue.  if a connection gets freed up at just
about the same time, the image request could go out before the priority 5 items.
 that seems like a problem to me.
I just didn't realize the queue processing was off the main thread.  No
forgetting involved.  ;)
Attached patch v2 patch — — Splinter Review 
ok, here's the revised patch.  i added a bumpPriority method to
nsISupportsPriority to simplify the task of incrementing or decrementing the
priority of an object.
Attachment #171611 - Attachment is obsolete: true
Attachment #174742 - Flags: superreview?(bzbarsky)
Attachment #174742 - Flags: review?(cbiesinger)
Comment on attachment 174742 [details] [diff] [review]
v2 patch

The new idl is missing... want to attach that too?

>Index: uriloader/base/nsDocLoader.cpp
>+NS_IMETHODIMP nsDocLoader::GetPriority(PRInt32 *aPriority)
>+{
>+  if (mLoadGroup) {
>+    nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mLoadGroup);

do_QI is null-safe, so no reason to check mLoadGroup, since we null-check |p|.

Similar for the other methods added to this class.

sr=bzbarsky with that, though I still want to see the idl.
Attachment #174742 - Flags: superreview?(bzbarsky) → superreview+
Sorry about that.  Here's the IDL file.
Comment on attachment 174742 [details] [diff] [review]
v2 patch

netwerk/base/src/nsLoadGroup.cpp
+    mPriority += aDelta;
+    if (aDelta != 0)
+	 PL_DHashTableEnumerate(&mRequests, RescheduleRequests, &aDelta);

nit: why not move the += into the if as well?

netwerk/protocol/http/public/nsIHttpChannelInternal.idl
+[scriptable, uuid(6f225cf9-dfeb-4b9f-8ff5-2a5bf31b73a1)]

looks like this is still different from the old IID of this interface :-)

netwerk/protocol/http/src/nsHttpChannel.cpp

nit: it seems the implementation of Bump/SetPriority would be slightly simpler
if BumpPriority called SetPriority and that did the work (no need to call
value-mPriority that way)

of course this way is more consistent with nsLoadGroup...

modules/libpr0n/src/imgLoader.cpp
+    p->BumpPriority(priority);

this is Bump rather than Set because of loadgroups?

hm, unrelated thought: nsISupportsPriority.idl does not say what the initial
value is. should it? or, should it say explicitly that it is left to the
implementation?

modules/libpr0n/src/imgRequest.cpp
+  // only the first proxy is allowed to modify the priority of this image
load.

hm... given that NewImageChannel now sets the priority, is this still needed?

Ah, this is for the image frame's BumpPriority?

modules/libpr0n/src/imgRequest.h
+  // Return the priority of the underlying network request, or return 0 if it

nit: maybe PRIORITY_NORMAL rather than 0?

uriloader/base/nsDocLoader.cpp
+  if (mLoadGroup) {
+    nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mLoadGroup);

nit: do_QI is nullsafe... no need for the if

So, it seems that new children of a docloader will not get a priority set on
them. maybe that should be fixed. though, putting docloaders in loadgroups
would fix this automatically.
(hm... I may have mentioned this already somewhere...)
Attachment #174742 - Flags: review?(cbiesinger) → review+
another thought: What about CSS files, scripts, etc? shouldn't they also get a
lower priority?

Shouldn't link click loads get a higher priority?

maybe that's better left for another patch.
I've been thinking about how to prioritize image loads more efficiently, and
think it would help if these were a bit further apart:  like -200, -100, 0, 100,
200.  It would let me give a bit more meaning to the actual image priority and
let me increase it more frequently (without having to clamp or put artifical
division in0 without worrying about hitting highest very quickly.

+  const long HIGHEST = -20;
+  const long HIGH    = -10;
+  const long NORMAL  =   0;
+  const long LOW     =  10;
+  const long LOWEST  =  20;
One other thought I had....  If docloader exposes the interface via GetI but not
QI, then once we have loadgroups in loadgroups we can just have GetI return the
loadgroup.  That would avoid the need for "legacy" glue code at that point...

Stuart, the problem is that the interface was considered as also a possible
interface to "renice", and that clamps values to -20 to 20....  Perhaps that's
not an issue?  Or perhaps we should say that implementations may scale, with
remainders dropped, rather than clamping if the implementation does not support
the full long range?  Then we could honestly set PRIORITY_HIGHEST to
PR_INT32_MAX and such, too.
One other thing.  Are the priority setting methods generally allowed to throw? 
The wording about priority changes being disallowed makes it sound like a
specific error will get thrown then...
bz, biesi: thanks for reviewing the patch.  lot's of good feedback.


(In reply to comment #41)
> another thought: What about CSS files, scripts, etc? shouldn't they also get a
> lower priority?
> 
> Shouldn't link click loads get a higher priority?
> 
> maybe that's better left for another patch.

right, i want to give link clicks slightly higher priority, but i'm saving that
for another patch.  the current patch still helps because inline images are
given lower priority.



(In reply to comment #42)
> I've been thinking about how to prioritize image loads more efficiently, and
> think it would help if these were a bit further apart:  like -200, -100, 0, 
> 100, 200.  It would let me give a bit more meaning to the actual image 
> priority and let me increase it more frequently (without having to clamp or 
> put artifical division in0 without worrying about hitting highest very 
> quickly.

why do you need so much granularity?  i think we could get away with classifying
images into maybe 3 priority buckets at most, and that would be just fine.


(In reply to comment #43)
> One other thought I had....  If docloader exposes the interface via GetI but 
> not QI, then once we have loadgroups in loadgroups we can just have GetI 
> return the loadgroup.  That would avoid the need for "legacy" glue code at 
> that point...

I think this is a non-issue.  We'll have maybe one consumer of this interface
via docloader, and that'll be the tabbrowser code.  We can change any of this at
any time.  We can make the tabbrowser code access nsISupportsPriority via GI,
but why bother when we control both the consumer and the implementation?
(In reply to comment #40)
> modules/libpr0n/src/imgLoader.cpp
> +    p->BumpPriority(priority);
> 
> this is Bump rather than Set because of loadgroups?

yes, and because the channel may impose some other default priority for whatever
reason.  it's better to code offsets than absolutes, i think.


> hm, unrelated thought: nsISupportsPriority.idl does not say what the initial
> value is. should it? or, should it say explicitly that it is left to the
> implementation?

hmm... perhaps it should say that the initial value is implementation specific.
 clearly, imgRequest sets its initial priority value in an interesting way.


> modules/libpr0n/src/imgRequest.cpp
> +  // only the first proxy is allowed to modify the priority of this image
> load.
> 
> hm... given that NewImageChannel now sets the priority, is this still needed?
> 
> Ah, this is for the image frame's BumpPriority?

YES :)  if the same image appears 100 times on the page, we don't need to give
it a 100x priority boost.  though pavlov made the point that it should probably
get some priority boost, we probably still want it to take second-seat to link
clicks and such.


> So, it seems that new children of a docloader will not get a priority set on
> them. maybe that should be fixed. though, putting docloaders in loadgroups
> would fix this automatically.
> (hm... I may have mentioned this already somewhere...)

Yeah, i'm not going to try to fix this just yet.  I want to wait to deal with
this issue when I start implementing the code in tabbrowser.
One other thing:  I think it is not important to be precise in how
Set/BumpPriority may fail.  I want this interface to be useful in a variety of
applications, and for this application at least, there's no point to reporting
errors when Set/BumpPriority cannot do what they are asked to do.  For example,
if HTTP has already scheduled the request for the highest priority,
BumpPriority(-1) would be a no-op.  Should we throw an exception in that case? 
Probably not.  I think this rule applies fairly well to all uses that I can
imagine.  Priority values are advisory.  The implementation can choose to do
whatever it likes with the given information.  Sound fair enough?
Sounds fine (re: comment 47); I think we should just clarify the api to say that
implementations should not throw exceptions when priorities are changed. The
current API comment makes it sound like an ok thing to do if you feel like it...
landed revised patch on the trunk, marking fixed.  i'll deal with tabbrowser and
such in a new bug.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
> Sounds fine (re: comment 47); I think we should just clarify the api to say that
> implementations should not throw exceptions when priorities are changed. The
> current API comment makes it sound like an ok thing to do if you feel like it...

But exceptions are always possible when it comes to XPCOM methods.  Anyways, I
went the other way and said that implementations may throw or not throw.
Exceptions are possible, yes.  The question is whether the particular case of
"changing the priority of an object may be disallowed" is allowed (or forced) to
throw, and if so whether we want to define what it should throw.  I see that
you've clarified that it's allowed to throw.  I'd prefer we clarify that it must
throw, and which exact exception it will throw (NS_ERROR_NOT_AVAILABLE?).
I think that's overkill.  I don't know of any use cases for that exception. 
None of the implementations throw, and none of the consumers check for
exceptions.  That doesn't mean that the interface couldn't be used that (i.e.,
if in prioritizing threads instead of network requests it makes sense for there
to be an exception thrown, then we can invent such an exception in the context
of threads).
It's interesting to note that this patch had no impact on our Tp tests.
Per request from shaver and brendan over IRC, I changed BumpPriority to
AdjustPriority.
Is this being actively exploited yet?  I'm not seeing any particularly obvious
change/improvement in prioritisation on a current trunk build.
Blocks: 47661
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: