336017 - keyword.enabled doesn't work in Camino (1.8branch and trunk)

Status: VERIFIED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Marc Gelfo]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20060426 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20060426 Camino/1.0+

I updated to the nightly build and now keyword.enable doesn't work!  I Get "This URL is not valid and cannot be loaded"

Reproducible: Always

Steps to Reproduce:
1.  Set keyword.enable to true in about:config
2.  Go to location bar and type search string, e.g. "britney spears"
3.  Cry because you don't get Britney Spears listings on Google

Actual Results:  
"This URL is not valid and cannot be loaded"

Expected Results:  
Keyword you typed should be sent to the search engine specified in keyword.URL

Comment 1

[Stuart Morgan]

Does it work in the corresponding Firefox build?  My guess would be not, since Camino doesn't do anything specific to support the pref.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If you type a single-word phrase, you get the normal fixup behavior, but I can confrim this is broken on the trunk.  

The root cause seems to be bug 331522, which changed the behavior, disabled keyword.enabled entirely for all apps, and then enabled it for Fx.  We depend on the xpfe/suite hookup, bug 332668, to get it working again.

Comment 3

[[e-mail]]

(In reply to comment #2)
>We depend on the xpfe/suite hookup, bug 332668, to get it working again.
I don't think you do, because you're an embedder. Instead you probably need to call nsIWebNavigation::LoadURI with the allow third party fixup flag.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> I don't think you do, because you're an embedder. Instead you probably need to
> call nsIWebNavigation::LoadURI with the allow third party fixup flag.

That comes up with no hits for Camino in LXR, either (Stuart, what did you search for last night when you said it seemed like we were dependent?)...

Comment 5

[Stuart Morgan]

I was looking for keyword.enable; Camino never reads that value.  So before the changes Camino didn't have to explicitly support it, but now it does?

Smokey, Camino calls LoadURI here:
http://lxr.mozilla.org/mozilla/source/camino/src/embedding/CHBrowserView.mm#422

Comment 6

[[e-mail]]

Now it has to explicitly force LoadURI to support it.

Comment 7

[froodian (Ian Leue)]

Attachment: A Start

This puts the support for the third party fixup flag in CHBrowserView, but

if (flags & NSLoadFlagsAllowThirdPartyFixup)

is never true (regardless of the pref's state).  Is this not working, or am I merely overlooking something obvious?

Comment 8

[Stuart Morgan]

Those code blocks in loadURI:... are just wrappers to make the call sites in the Camino code not need to know about core contant values.  Adding that code will just make it so that if loadURI:... were explicitly called with NSLoadFlagsAllowThirdPartyFixup it would be passed along--but of course nothing is calling it that way since it didn't exist.

If I'm understanding neil correctly, what you want is:

if (<keyword.enable is on>)
  navFlags |= nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;

(And there would be no need to wrap the constant for Camino use.)

There's a larger question of whether Camino wants to take advantage of the new ability to selectively disable fixup where it doesn't make sense to do, but since it's hidden and disabled by default, just getting the old behavior back is good enough for now; evaluating each call to loadURI:... isn't worth the trouble.

Comment 9

[froodian (Ian Leue)]

Attachment: Patch

Per Pink and IRC, we should be doing this at a higher level than CHBrowserView.

Comment 10

[Stuart Morgan]

Comment on attachment 227140 [details] [diff] [review]
Patch

r=me

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227140 [details] [diff] [review]
Patch

sr=pink

Comment 12

[Nick Kreeger]

Fixed trunk and branch.

Comment 13

[benc]

V/fixed.

works in camino 1.5.

haven' tested the trunk, but did check lxr.