Closed Bug 52510 Opened 24 years ago Closed 23 years ago

HTTP handler should register as a listener for user agent prefs changes.

Categories

(Core :: Networking: HTTP, defect, P1)

Product:
Core
Component:
Networking: HTTP
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.4

People

(Reporter: jud, Assigned: darin.moz)

References

Details

(Keywords: topembed, Whiteboard: fixed-on-trunk [PDT+])

Attachments

(2 files)

v1.0 patch, cleans up and completes http pref handling
22.81 KB, patch
jud
: review+
mscott
: superreview+
Details | Diff | Splinter Review
patch for the 0.9.4 branch, including leak fixes
26.81 KB, patch
bnesse
: review+
rpotts
: superreview+
Details | Diff | Splinter Review 
the HTTP protocol handler should listen to changes in useragent prefs.
Futuring this...
Target Milestone: --- → Future
http bugs to "Networking::HTTP"
Assignee: gagan → darin
Component: Networking → Networking: HTTP
Target Milestone: Future → M19
Target Milestone: --- → Future
Target Milestone: Future → mozilla1.0
Added Topembed keyword. 
Mozilla not paying attention to the changes in useragent preferences. This
can cause wrong user agent on the Embedded app startup (on new user profiles only).
Keywords: topembed
-> mozilla0.9.5
Target Milestone: mozilla1.0 → mozilla0.9.5
Keywords: nsbranch
Priority: P3 → P1
Status: NEW → ASSIGNED
why is this necessary?  we already listen for general.useragent.override.  why
would different profiles need to modify portions of the default useragent string?
There aren't any comments on this bug since the 17th of
Sept.  Can QA regess against the Netscape commercial builds and determine if
this is still a valid bug?  If so, and we can get fixes/reviews in the next day
or two, please mark as nsbranch+ which will get this on the PDT radar. Else,
mark is as nsbranch-. Also, can someone comment in the bug how serious you think
this is?  PDT is only accepting "stop ship" bugs such as data loss and loss of
major functionality.
override was an afterthought. the need here is in individual component
modification. this bug was created long before the override stuff was put in,
and IMO, is still valid. 

caching the whole string and parsing in/out components will lead to folks
banking on the format of the string (which as history tells us, isn't good). the
format should be left up to impl to deal w/, and consumers should just have to
worry about their component.
ok, but nsIHttpHandler allows any component the freedom to change the UA parts.
why do we want to be able to program the UA parts via prefs?
Because nsIPrefService is an API which is exposed to embeddors and
nsIHttpHandler isn't. Unless we want to freeze nsIHttpHandler and expose it but
I bet we don't.
ok.. that definitely makes sense.  then does this need to be nsbranch?
nsbranch and topembed are synonymous right now I think. topembed means drive it
into the 0.9.4 branch. I'll PDT+ it.
Whiteboard: PDT+
nsbranch -> nsbranch+
Keywords: nsbranch → nsbranch+
so i understand this to mean that the http handler should simply listen to pref
changes on anything matching "general.useragent." ... correct?

attaching a patch to make it so.
Yeah - just use the nsIPrefBranchInternal::AddObserver() stuff instead of the
old C callbacks. I bet you knew that but just making sure ;-)
yeah, i'm going to try to completely cleanup the pref code in http.
jud: can you review this?
cc'ing bnesse so he can see this too.

Brian, why does Darin need the "internal" interface here? IIRC, it's because we
don't expose observation publically; right?

Looks good to me. My nested #define macro know-how is rusty though, your #define
PREF_CHANGED() macro refers to "pref" but I don't see that declared anywhere.
Does your UA_PREF() "_pref" get translated to "pref" when nested?

If the answer is yes to my above question, r=valeski.
Jud, yes that's correct. The Observation stuff was placed into the internal
interface because there were questions as to whether or not the Observer
interface was going to be exposed to embedding IIRC.

The patch looks great. Thanks darin for wacking one off my list of interfaces
that need to be cleaned up.
Attachment #50859 - Flags: review+
jud: pref is the argument to PrefsChanged... _pref is the argument to the UA_PREF
macro... they are not the same thing.
thanks for clarification.
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling

sr=mscott
Attachment #50859 - Flags: superreview+
pulling branch '+', because this isn't needed for eMojo. but leaving topembed
because we'll need this on 0.9.4 after 6.2 is done.
Keywords: nsbranch+ → nsbranch-
removing PDT - The PDT
Whiteboard: PDT+
fixed-on-trunk
Whiteboard: fixed-on-trunk
-> mozilla0.9.4 for checkin on the branch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
This checkin caused thi tinderbox leak stats to increase: see bug 102332.
has this hit the 0.9.4 branch yet?
nope
pls check this into the branch - PDT+, once you have good confidence.
Whiteboard: fixed-on-trunk → fixed-on-trunk [PDT+]
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling

r=rpotts@netscape.com

Looks good to me.  The only thing that i noticed was that the UA_PREF.misc was checked for twice :-)

you might want to remove the extra one...

-- rick
Comment on attachment 52810 [details] [diff] [review]
patch for the 0.9.4 branch, including leak fixes

Looks good. r=bnesse.
Attachment #52810 - Flags: review+
Blocks: 64833
Comment on attachment 52810 [details] [diff] [review]
patch for the 0.9.4 branch, including leak fixes

sr=rpotts@netscape.com

this patch makes my eyes cross :-)
Attachment #52810 - Flags: superreview+
fixed-on-0.9.4-branch
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
* useragent string for version (misc) appears in console window.
* useragent string for locale didn't appear in the console, but Darren explained
this will appear in the user agent.
* Accept-Language entries are listed and in correct order.
* Accept-Charset entry is listed, also 0.66 "other" entry.
* User-Agent string incls app name, version, platform, OS: "Mozilla", "5.0",
"Windows", "WinNT4.0"
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: