Closed Bug 383716 Opened 15 years ago Closed 14 years ago

nsWebBrowserPersist::OnStopRequest doesn't check |status| for failure

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: Mardak, Assigned: Mardak)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
It's possible to get a status failure OnStop, but |status| is only used when passing to |mProgressListener|.

Perhaps do something similar to exthandler's OnStop?

http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2129

If there is a failure, a the |mProgressListener| will get a StatusChange with the status and error string; otherwise, it'll get a StateChange (stop) like usual.

Question is.. how many failures are actually arriving OnStop that didn't appear OnDataAvail.
Attachment #267701 - Flags: review?(cbiesinger)
Assignee: nobody → edilee
This is necessary for the download manager to get the Link Fingerprint failure status as a StateChange.
Status: NEW → ASSIGNED
Attached patch v2 (obsolete) — Splinter Review
- Fix nits (comments, void)
Attachment #267701 - Attachment is obsolete: true
Attachment #267701 - Flags: review?(cbiesinger)
Edward, you'll request reviews once you're happy with the patch, right?
Attachment #269539 - Attachment description: v2 (trunk only) → v2
Attachment #269539 - Flags: superreview?(cbiesinger)
Attachment #269539 - Flags: review?
Comment on attachment 269539 [details] [diff] [review]
v2

- I think it makes more sense to call EndDownload instead of Cancel
- don't cast to void
- you need to continue in this function, because Cancel does not set mEventSink etc to null
Attachment #269539 - Flags: superreview?(cbiesinger)
Attachment #269539 - Flags: review?
Attachment #269539 - Flags: review-
Attached patch v3 (obsolete) — Splinter Review
(In reply to comment #4)
> - you need to continue in this function, because Cancel does not set mEventSink
> etc to null
Done. Instead of returning, it'll reuse the existing code to |delete data| which will get |completed| set to true allowing us to...

> - I think it makes more sense to call EndDownload instead of Cancel
... reuse the EndDownload + nulling. Done.

> - don't cast to void
Removed.
Attachment #269539 - Attachment is obsolete: true
Attachment #273096 - Flags: review?(cbiesinger)
Attached patch v4Splinter Review
Check if we haven't gotten a failure Result instead of checking if we haven't been canceled yet.
Attachment #273096 - Attachment is obsolete: true
Attachment #273208 - Flags: review?(cbiesinger)
Attachment #273096 - Flags: review?(cbiesinger)
Attachment #273208 - Flags: superreview?(bzbarsky)
Attachment #273208 - Flags: review?(cbiesinger)
Attachment #273208 - Flags: review+
Comment on attachment 273208 [details] [diff] [review]
v4

Seems reasonable.
Attachment #273208 - Flags: superreview?(bzbarsky) → superreview+
Keywords: checkin-needed
Checking in embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp;
/cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v  <--  nsWebBrowserPersist.cpp
new revision: 1.130; previous revision: 1.129
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta1
Flags: in-testsuite?
Depends on: 426409
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.