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

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Location Bar & Autocomplete
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Marc Gelfo, Assigned: froodian (Ian Leue))

Tracking

({verified1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
verified1.8.1

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.75 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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

12 years ago
Does it work in the corresponding Firefox build?  My guess would be not, since Camino doesn't do anything specific to support the pref.
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.
Status: UNCONFIRMED → NEW
Depends on: 332668
Ever confirmed: true
Summary: keyword.enable doesn't work → keyword.enabled doesn't work in Camino (1.8branch and trunk)

Comment 3

11 years ago
(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.
(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?)...
Keywords: regression
Target Milestone: --- → Camino1.1

Comment 5

11 years ago
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
No longer depends on: 332668
Whiteboard: [good first bug]

Comment 6

11 years ago
Now it has to explicitly force LoadURI to support it.
(Assignee)

Comment 7

11 years ago
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?
Assignee: nobody → stridey
Status: NEW → ASSIGNED

Comment 8

11 years ago
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.
(Assignee)

Comment 9

11 years ago
Created attachment 227140 [details] [diff] [review]
Patch

Per Pink and IRC, we should be doing this at a higher level than CHBrowserView.
Attachment #226039 - Attachment is obsolete: true
Attachment #227140 - Flags: review?(stuart.morgan)

Comment 10

11 years ago
Comment on attachment 227140 [details] [diff] [review]
Patch

r=me
Attachment #227140 - Flags: superreview?(mikepinkerton)
Attachment #227140 - Flags: review?(stuart.morgan)
Attachment #227140 - Flags: review+
Comment on attachment 227140 [details] [diff] [review]
Patch

sr=pink
Attachment #227140 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [good first bug] → [good first bug] [needs checkin]

Comment 12

11 years ago
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: regression → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [good first bug] [needs checkin]

Comment 13

10 years ago
V/fixed.

works in camino 1.5.

haven' tested the trunk, but did check lxr.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
QA Contact: location.bar → benc
You need to log in before you can comment on or make changes to this bug.