Closed Bug 162845 Opened 23 years ago Closed 23 years ago

PPEmbed needs native impl of download progress UI

Categories

(Core Graveyard :: Embedding: Mac, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: ccarlen, Assigned: ccarlen)

Details

Attachments

(4 files, 2 obsolete files)

Currently, it's using the same XUL impl as mozilla.
Taking.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
Just want to make sure: not only will you be replacing the xul with native, but you will also be using the new native progresslistener for webpersist operations like "Save page as..." and "Save link as...".
Attached patch patch (obsolete) — Splinter Review
Simon did CHeaderSniffer.cpp/.h, UCustomNavServicesDialogs.cpp/.h and most of the changes to CBrowserShell.cpp.
Attached file changed resource files (obsolete) —
Looking for review. I'm now going to make a better icon for the close pane control, but that won't affect the diffs.
+ if (LEventDispatcher::GetCurrentEventDispatcher()) { // Can this ever be NULL? + EventRecord theEvent; + while (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil)) + LEventDispatcher::GetCurrentEventDispatcher()->DispatchEvent(theEvent); + } this seems a tad skanky. where did this code come from? was it always there? Why are the files called UDownload.cpp/.h when the class in them is CDownload? You do this in a couple of places. + // An download is beginning. Initialize the UI for this download. Nit. Should read "A download". + Boolean cmdHandled = false; + + if (!cmdHandled) + cmdHandled = LWindow::ObeyCommand(inCommand, ioParam); + + return cmdHandled; How about just: return LWindow::ObeyCommand(inCommand, ioParam); and if you're going to do that, why have this routine at all? + const float kOneThousand24 = 1024.0; oh now this is just plain silly ;) ;) there's a lot here, it's hard to wrap my head around all of the relationships. how well have you tested things like closing the dialog while it's in the middle of downloading, cancelling one d/l while others are still going on, etc? It took a while to get all the ownership bugs weeded out of the chimera implementation, especially with things like quitting the app while downloading. tenative r=pink.
> this seems a tad skanky. where did this code come from? was it always there? I had to add it for this. The problem is that the save dialog goes away, which generates an activate event for the window behind it. Then, the download progress dialog is opened before returning to the main event loop and that generates another set of activate events. Processing the pending activate/updates immediately after dismissing the dialog fixes this. > Why are the files called UDownload.cpp/.h when the class in them is CDownload? There's also CHelperAppLauncherDialog in that file. I like to begin file names of files implementing more than 1 object with "U" for "Unit." Sorry, my MacApp roots are showing. > and if you're going to do that, why have this routine at all? Whoops - yeah that method needs to go away. > + const float kOneThousand24 = 1024.0; oh now this is just plain silly ;) ;) Do you know that if you type 1024.0 n times in a function, that there are not n 8-byte globals in your PEF? Since I wasn't completely sure, I used 1 const float. > how well have you tested things like closing the dialog while it's in the > middle of downloading, cancelling one d/l while others are still going on Quite well. I put code in to alert the user when closing the window on an in-progress download that it will cancel the download. Same thing on quitting the app.
Attached patch updated patchSplinter Review
Cleaned up a few things Pink pointed out, especially the empty ObeyCommand() Made a 32-bit icon for the "close pane" control and some nescesary changes to CIconServicesIcon. Got rid of CMultiDownloadProgress::HandleAppleEvent. This is a big comment at the top of UDownloadDisplay explaining it.
Attachment #96756 - Attachment is obsolete: true
Attached file updated resources
Attachment #96757 - Attachment is obsolete: true
Attached image screenshot
Also added in the updated patch: getting the font styles for the static text items in the dialog with GetThemeFont(). Here's a screenshot of it.
just a ui suggestion: i'd rather see the gray (x) like used in projectBuilder and the search text field in jaguar rather than the red circle to close a particular panel.
A diff between the two patches for the sake of Pink who reviewed the first one. It's a little hard to understand, but hopefully easier than reviewing the new patch from scratch
Comment on attachment 96906 [details] [diff] [review] updated patch r=pink
Attachment #96906 - Flags: review+
Comment on attachment 96906 [details] [diff] [review] updated patch sr=sfraser
Attachment #96906 - Flags: superreview+
Comment on attachment 96906 [details] [diff] [review] updated patch a=chofmann for 1.0.2
Attachment #96906 - Flags: approval+
Checked into branch. Leaving open until it's in the trunk.
Keywords: fixed1.0.2
QA Contact: mdunn → depstein
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Please verify the bug. Once verified, change the keyword fixed1.0.2 to verified1.0.2
QA Contact: depstein → dsirnapalli
-- Marking as verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: