Closed Bug 381149 Opened 13 years ago Closed 2 years ago

implement GetURIGeckoFlags / SetURIGeckoFlags in places

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: moco, Unassigned)

References

Details

Attachments

(1 file)

implement GetURIGeckoFlags / SetURIGeckoFlags in places

see Bug 293714, where roc made us "be smart about guessing whether a vertical scrollbar is needed when we do the first real reflow of a scrollframe; remember in global history whether the page needed a scrollbar or not."

from the places, we have:

//    FIXME: should we try to use annotations for this stuff?

in some other bug (a places perf bug?) boris talks about how we might want to avoid making this call for certain uri (res:/, I think).

here's a stack to where it is called:

>	places.dll!nsNavHistory::GetURIGeckoFlags(nsIURI * aURI=0x03bb4300, unsigned int * aResult=0x0012e9e8)  Line 2977	C++
 	gklayout.dll!nsGfxScrollFrameInner::GetVScrollbarHintFromGlobalHistory(int * aVScrollbarNeeded=0x0012e9fc)  Line 2664 + 0x21 bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::GuessVScrollbarNeeded(const ScrollReflowState & aState={...})  Line 503 + 0xf bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::ReflowContents(ScrollReflowState * aState=0x0012eb60, const nsHTMLReflowMetrics & aDesiredSize={...})  Line 536 + 0xf bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::Reflow(nsPresContext * aPresContext=0x04001a48, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 752 + 0x13 bytes	C++
 	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x0405d4fc, nsPresContext * aPresContext=0x04001a48, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0, int aY=0, unsigned int aFlags=0, unsigned int & aStatus=0)  Line 672 + 0x21 bytes	C++
 	gklayout.dll!ViewportFrame::Reflow(nsPresContext * aPresContext=0x04001a48, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 285 + 0x2b bytes	C++
 	gklayout.dll!PresShell::DoReflow(nsIFrame * target=0x0405d324)  Line 6006	C++
 	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=0)  Line 6114	C++
 	gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Layout)  Line 4313	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 1026	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x03f12604, nsIChannel * aChannel=0x03f15aa8, unsigned int aStatus=0)  Line 4863	C++
 	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x03f12604, nsIChannel * channel=0x03f15aa8, unsigned int aStatus=0)  Line 998	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x03f12604, nsIRequest * aRequest=0x03f15aa8, unsigned int aStateFlags=131088, unsigned int aStatus=0)  Line 4778	C++
 	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x03f12604, nsIRequest * aRequest=0x03f15aa8, int aStateFlags=131088, unsigned int aStatus=0)  Line 1236	C++
 	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x03f15aa8, unsigned int aStatus=0)  Line 869	C++
 	docshell.dll!nsDocLoader::DocLoaderIsEmpty()  Line 765	C++
 	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x03f15aa8, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 682	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x03f15aa8, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 676 + 0x2e bytes	C++
 	necko.dll!nsBaseChannel::OnStopRequest(nsIRequest * request=0x03f15b60, nsISupports * ctxt=0x00000000, unsigned int status=0)  Line 630	C++
 	necko.dll!nsInputStreamPump::OnStateStop()  Line 571	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x03f16600)  Line 395 + 0xb bytes	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::Run()  Line 112	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f98c)  Line 483	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bae4e8, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	xpcom_core.dll!nsThread::Shutdown()  Line 442 + 0xb bytes	C++
 	necko.dll!nsSocketTransportService::Shutdown()  Line 413	C++
 	necko.dll!nsIOService::SetOffline(int offline=1)  Line 628 + 0x1c bytes	C++
 	necko.dll!nsIOService::Observe(nsISupports * subject=0x03d99ca8, const char * topic=0x1003585c, const unsigned short * data=0x10035fe8)  Line 786	C++
 	xpcom_core.dll!nsObserverList::NotifyObservers(nsISupports * aSubject=0x03d99ca8, const char * aTopic=0x1003585c, const unsigned short * someData=0x10035fe8)  Line 129	C++
 	xpcom_core.dll!nsObserverService::NotifyObservers(nsISupports * aSubject=0x03d99ca8, const char * aTopic=0x1003585c, const unsigned short * someData=0x10035fe8)  Line 184	C++
 	xul.dll!nsXREDirProvider::DoShutdown()  Line 750	C++
 	xul.dll!ScopedXPCOMStartup::~ScopedXPCOMStartup()  Line 791	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00ba9dc0, const nsXREAppData * aAppData=0x004036e0)  Line 2871	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00ba9dc0)  Line 65 + 0x13 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
Assignee: nobody → sspitzer
Flags: blocking-firefox3?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 3
Attached file patch for layout
as pointed out by boris (in his only "startup performance testing"), we don't need to Get/Set the gecko flags for certain URIs.

On startup, we do:

resource://gre/res/hiddenWindow.html
about:blank
http://myhomepage.com

Next up, actually implemente Get/Set gecko flags as annotations.
Attachment #274794 - Flags: review?(bzbarsky)
Comment on attachment 274794 [details]
patch for layout

>Index: nsGfxScrollFrame.cpp
>+  (void)uri->SchemeIs("http", &shouldSetGeckoFlags);

No need for the (void) part.

And are we sure we don't want this for, say jar:http, etc?  I don't like hardcoded scheme checks here; protocol flags would be better if we want to do this.

Of course, we shouldn't be reflowing the hidden window's HTML, imo.  

>+  NS_ENSURE_SUCCESS(rv = uri->SchemeIs("http", &shouldCheckGeckoFlags), rv);

Why suddenly check rv here?  I wouldn't bother, as above...

Can you point me to this "startup performance testing" thing?  I'd like to recall what I was looking at then.
bz:  thanks for the prompt review.

some questions:

1) "protocol flags would be better"

Is there a particular flag (or combination of flags) you recommend, or should I add one?  Adding one (and only setting it nsHttpHandler::GetProtocolFlags()) sounds cleaner.  (note, nsIProtocolHandler is frozen)

2)  "if we want to do this"

I think we want to do this.  in addition to delaying instantiation of the history service (and delaying import), it also saves us from the calling into places to get / set the gecko flags (saving us a SELECT and UPDATE work in the database.)

3) "Of course, we shouldn't be reflowing the hidden window's HTML, imo."

should I spin that off as another bug:  "don't reflow the hidden window"

4) "Why suddenly check rv here?  I wouldn't bother, as above..."

ok, I'll remove all un-needed rv checks.

5) "Can you point me to this "startup performance testing" thing?"

see bug #380134 comment #6 (it was from your comments / findings that I logged this bug.)

while I wait for your responses / suggestions, I'll go implement the other half of this bug:  Get/SetGeckoFlags using annotations.
Status: NEW → ASSIGNED
Just adding a protocol flag would be the way to go if we want to do that.

I guess what really bothers me is that this is building knowledge of the history implementation details into Gecko.  It would make a lot more sense to me to do the checks in the history impl, but unfortunately it's instantiation of the history that does the import, not the first history access, right?

Not reflowing the hidden window should indeed be a separate bug.

I still think it makes more sense to check for a chrome docshell (and maybe a XUL document) and bail out if we're in one.  That would solve the hidden window case without building knowledge of the history into layout, slowing down rendering of FTP listings, etc, etc.

roc, thoughts?
The Gecko flag we're using didn't give us much performance boost, IIRC. If implementing this on places is going to be expensive (for performance), we probably should just drop it.
roc / bz:  I could attempt a fix (with the less-than-idea scheme check), and see if it is worth it (using tinderbox or other performance tools to measure).

if it seems worth it:  

finish the fix by adding a new protocol flag to nsIProtocolHandler or by checking if we are in a chrome docshell / xul document.

if not:  remove the calling code in layout and the stubs in history
That seems reasonable.
Comment on attachment 274794 [details]
patch for layout

r+ with my nits addressed, at least as an interim solution.
Attachment #274794 - Flags: review?(bzbarsky) → review+
If this isn't a huge perf win, I don't think we should block on it, especially if the cost of implementing on Places takes away from the benefit in layout. :)
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Target Milestone: Firefox 3 → ---
Assignee: moco → nobody
Status: ASSIGNED → NEW
Blocks: 510119
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
AFAICT we don't have gecko flags now.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.