181764 - make page info appear faster

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, enhancement)

Description

[Daniel Brooks [:db48x]]

This is just a bug where I'm going to play with some code and see what people think.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: processes the page in the background

patch is funny, but seems to work

Comment 2

[Daniel Brooks [:db48x]]

Attachment: -uw

maybe the line endings changed? I dunno.

Anyway what this does is send the processing to another thread, so it can
process without blocking the UI. It even updates a progress meter, though I
feel like that's a little silly. It's supposed to trap events that focus the
tabs, so that it can focus them after the processing is done, but for some
reason it can't cancel the events. Maybe instead of that I should call
rowCountChanged() on the view for each row I add, that should make the tree
redraw it's scrollbars and stuff, so you'll be able to see something happening
that way.

At this point, it's just a cool idea for me to play with on a weekend.

Comment 3

[Daniel Brooks [:db48x]]

Attachment: process the page in the background, and also update the trees

it works, and it doesn't really slow it down. Either way (update the trees live
or progress meter) are, in my opinion, better than sitting there not doing
anything but eating cpu for 15 seconds.

Comment 4

[Daniel Brooks [:db48x]]

cc'ing some people whose opionions I desire. Hixie and I talked about doing this
a while back, and I think timeless and I talked about a progress meter a long
time ago. What do you guys think? Is this the right way to go about it? Is the
implementation good? Is it just me, or is the progress meter silly?

Comment 5

[Hixie (not reading bugmail)]

I like the idea of populating the dialog in the background while the first tab
is shown. It seems to make sense to me.

Comment 6

[Daniel Brooks [:db48x]]

what about the progress meter?

Comment 7

[Hixie (not reading bugmail)]

I'd need to see it to know, really. How about we try it for a while?

Comment 8

[Daniel Brooks [:db48x]]

Comment on attachment 107318 [details] [diff] [review]
process the page in the background, and also update the trees

well Jag? could I get it checked in for a while? this is the alpha we're
working on after all...

I think it's a pretty nifty patch.

Comment 9

[timeless]

Comment on attachment 107318 [details] [diff] [review]
process the page in the background, and also update the trees

why not use undefined, true, false?

Comment 10

[jag (Peter Annema)]

Your patch needs some clean-up (commented out code, global |var i| could use a
better and less generic-therefore-easy-to-clash name).

I like the approach of doing this work in the background instead of on demand,
since it's a lot of work, but there are certain aspects I don't like. One of
them is that a click on a tab is ignored if that tab isn't ready yet. Slightly
better would be to gray the tab out so you at least indicate it's not clickable,
slightly better than that is a small progress meter in the tab so you can tell
when it's done, and better than that would be to switch to that tab panel,
showing some message indicating that the information is being gathered, with a
progress meter to indicate how far you are.

Hmmm, you may not need a progress meter per se. I'm looking at the "Forms" tab,
and it takes forever to show up. Could you make it so that it shows all the
fields, and then populates them so the user can actually see rows being added to
the outliners (perhaps in groups of 5 or 10)? This way the user gets adequate
feedback to their click on the tab, and they can see that work is being done,
the shrinking scrollbar thumb (or just visibly items being added if there's no
scrollbar yet) acting as a natural progress "meter".

Patrice, Lori, we could use some help here :-)

Comment 11

[jag (Peter Annema)]

Heh, I guess that last paragraph is what you meant with "updating the trees
live" (comment 3).

How hard would it be to make that work? I've talked to Patrice and she suggested
that if the "live updating" takes a while it might still be a good idea to add a
progress meter, perhaps accompanied by some text like "Processing document".

Comment 12

[Daniel Brooks [:db48x]]

this patch does update the trees live in addition to showing a progress meter.
It doesn't bother to batch rows in groups, though I suppose it could. It didn't
seem signifigantly slower in any case. I guess I'll have to get more objective
numbers on that though. Also, the part where I tried to ignore the click doesn't
actually work, though I have no idea why.

As far as forms taking longer to show up, that's usually because forms tend to
be farther down the page, and page info is processing the entire document in a
single pass through the dom tree to collect everything.

I'll clean it up and post it again soon.

Comment 13

[Daniel Brooks [:db48x]]

Attachment: pageinfo.js/.xul so someone can make me a diff

cleaned it up too

Comment 14

[timeless]

Attachment: as a diff

Comment 15

[Daniel Brooks [:db48x]]

Attachment: aha!

I figured out what I was doing wrong. Because in my implementation of
nsITreeView's addRow() I call rowCountChanged(), I don't need to call
rowCountChanged() in onTabClick(). I also cleaned up onTabClick() so that it
only modified the selection on the correct view, and to only select the first
row when nothing is already selected, to avoid changing the selection when the
user isn't expecting it. makeTabs() also no longer bothers to check if it's
already been called, since it's now called from the onload handler and not an
onclick handler that can fire multiple times.

and with that, I think it's ready for a real review. Have I forgotten anything?

Comment 16

[Daniel Brooks [:db48x]]

and sorry about the whole zip compression thing, I know it sucks

Comment 17

[timeless]

Attachment: as diff (as=>has, split long line)

Comment 18

[Daniel Brooks [:db48x]]

just as a note, I know that getElementsByTagName("*") isn't very nice. As a
result, I'll just remove that part and make the progress meter indeterminate,
which still conveys the most essential facts (still working/finished).

In the end, I rather think the progress meter will be removed anyway. It just
doesn't look right to me.

Anyway, that'll wait till tomorrow.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 108166 [details] [diff] [review]
as diff (as=>has, split long line)

You were going to post a new patch, no?

Comment 20

[Daniel Brooks [:db48x]]

opps, yea

Comment 21

[Daniel Brooks [:db48x]]

Attachment: indeterinate meter to avoid grabbing a list of all the nodes in the document

Comment 22

[Daniel Brooks [:db48x]]

Comment on attachment 110929 [details] [diff] [review]
indeterinate meter to avoid grabbing a list of all the nodes in the document

looking for r= and sr=

Comment 23

[Boris Zbarsky [:bzbarsky]]

I cannot review this in the foreseeable future (3 weeks, at least, maybe more).

Comment 24

[Daniel Brooks [:db48x]]

Comment on attachment 110929 [details] [diff] [review]
indeterinate meter to avoid grabbing a list of all the nodes in the document

what about you caillon?

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 110929 [details] [diff] [review]
indeterinate meter to avoid grabbing a list of all the nodes in the document

> pageInfoTreeView.prototype = {
>@@ -97,6 +96,7 @@
>   addRow: function(row)
>   {
>     this.rows = this.data.push(row);
>+    this.rowCountChanged(this.rows-1, this.rows);

Shouldn't the second arg be |1|?

>   },
> 
>   addRows: function(rows)
>@@ -104,6 +104,7 @@
>     var length = rows.length;
>     for(var i = 0; i < length; i++)
>       this.rows = this.data.push(rows[i]);
>+    this.rowCountChanged(length, this.rows);

Shouldn't this be |this.rowCountChanged(this.rows - length, length)| ?


> //******** Generic Build-a-tab
>+// assumes the views are empty and no longer checks to see if the work has already been done,
>+// because this is no longer called from an event handler that can be called more than once

Your comment should not reference what code used to do, but what it does. 
Remove the "no longer..." part.  Or if you want, re-word it to say something
like "We should not check ... because of ... " which makes it relevant to the
current code and acts as a warning to future developers who (may) think of
doing that.

Comment 26

[Daniel Brooks [:db48x]]

Attachment: fix caillon's three good suggestions

Comment 27

[Daniel Brooks [:db48x]]

Attachment: duh

hmm, using 1 (like you said) instead of this.rows - oldrows is probably better
:P

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 111151 [details] [diff] [review]
duh

>@@ -97,6 +96,7 @@
>   addRow: function(row)
>   {
>     this.rows = this.data.push(row);
>+    this.rowCountChanged(this.rows-1, 1);

I can't figure out if your style in this file needs a space in between the
subtraction operands or not...
>   },
> 
>   addRows: function(rows)


>@@ -457,11 +463,13 @@
> }
> 
> //******** Generic Build-a-tab
>+// assumes the views are empty. only called once to build the tabs, and
>+// does so by farming the task off to another thread via setTimeout().

How about using sentence case?



>+function doTabClick(view)

Maybe |ensureSelection| for the func name?


Looks fine otherwise, if its tested and works.

Comment 29

[Daniel Brooks [:db48x]]

Attachment: patch with those changes [Checked in: comment "30"]

thanks caillon

Comment 30

[jag (Peter Annema)]

Comment on attachment 111584 [details] [diff] [review]
patch with those changes
[Checked in: comment "30"]

sr=jag

Comment 31

[Daniel Brooks [:db48x]]

so, now that it's in, what do you guys think? With or without the progress meter?

Comment 32

[[e-mail]]

You can't leave the progress meter like that. It's stretched vertically for a
start. And I don't think the width is right; easiest way to fix that would be to
leave some flex on the spacer and have some on the progress meter as well, e.g.
<box align="center">
  <progressmeter flex="2" value="0" mode="determined"/>
  <spacer flex="1"/>
  <button label="Help"/>
</box>

Comment 33

[[e-mail]]

Oh, and a couple of other points now that I look:
use meter.hidden = true; in preference to meter.setAttribute("hidden", "true");
don't bother setting the other meter attributes if you're hiding it anyway.

Comment 34

[Daniel Brooks [:db48x]]

It should be noted that the whole point of the exercise is to determine if the
progress meter is even needed. All of the trees update in real time, so if it
looks garish (or worse, silly) then we can remove the meter. On the other hand,
if there's one form on a page and 1000 links before it, then it can take a while
for that form to appear in the forms tab.

So, is it needed?

Good point about setting all kinds of attributes and then hiding it. that is
kind of silly ;)

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 111584 [details] [diff] [review]
patch with those changes
[Checked in: comment "30"]

Didn't this patch already land, with my review?

Comment 36

[Daniel Brooks [:db48x]]

Comment on attachment 111584 [details] [diff] [review]
patch with those changes
[Checked in: comment "30"]

Yea, it did. My question still remains, however.

Comment 37

[Daniel Wang]

Can the <progressmeter> be hidden? I think we should remove
  <box>...<button label="&helpButton;" oncommand="doHelpButton();" /></box>
and put
  buttons="accept,help"
  ondialoghelp="doHelpButton();"
in the <window> tag, so people don't accidently hit Help, see bug 140466.

Comment 38

[Daniel Brooks [:db48x]]

has been fixed for a while