Last Comment Bug 336017 - keyword.enabled doesn't work in Camino (1.8branch and trunk)
: keyword.enabled doesn't work in Camino (1.8branch and trunk)
Status: VERIFIED FIXED
: verified1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Location Bar & Autocomplete (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
: benc
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-30 09:07 PDT by Marc Gelfo
Modified: 2007-07-08 22:55 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
A Start (1.83 KB, patch)
2006-06-17 22:34 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (3.75 KB, patch)
2006-06-26 13:48 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Marc Gelfo 2006-04-30 09:07:58 PDT
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 User image Stuart Morgan 2006-04-30 10:09:53 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-06 22:26:29 PDT
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 User image neil@parkwaycc.co.uk 2006-05-07 04:24:56 PDT
(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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-07 04:59:19 PDT
(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 User image Stuart Morgan 2006-05-07 09:00:55 PDT
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 User image neil@parkwaycc.co.uk 2006-05-11 09:10:34 PDT
Now it has to explicitly force LoadURI to support it.
Comment 7 User image froodian (Ian Leue) 2006-06-17 22:34:45 PDT
Created attachment 226039 [details] [diff] [review]
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 User image Stuart Morgan 2006-06-24 16:16:02 PDT
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 User image froodian (Ian Leue) 2006-06-26 13:48:15 PDT
Created attachment 227140 [details] [diff] [review]
Patch

Per Pink and IRC, we should be doing this at a higher level than CHBrowserView.
Comment 10 User image Stuart Morgan 2006-06-29 20:00:03 PDT
Comment on attachment 227140 [details] [diff] [review]
Patch

r=me
Comment 11 User image Mike Pinkerton (not reading bugmail) 2006-08-02 07:56:15 PDT
Comment on attachment 227140 [details] [diff] [review]
Patch

sr=pink
Comment 12 User image Nick Kreeger 2006-08-02 15:56:32 PDT
Fixed trunk and branch.
Comment 13 User image benc 2007-07-08 22:55:30 PDT
V/fixed.

works in camino 1.5.

haven' tested the trunk, but did check lxr.

Note You need to log in before you can comment on or make changes to this bug.