Closed Bug 988129 Opened 11 years ago Closed 11 years ago

Correct the chaotic naming styles in system message codes

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

Attachments

(4 files, 6 obsolete files)

Part 1, remove function name, V1.1
16.34 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
Part 2, s/manifest/manifestURL, V2.1
29.73 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
Part 3, s/uri/pageURL, V2.1
23.56 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
Part 4, alignment, V1.1
10.44 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
Our naming styles are really messy and chaotic. Really hope to correct all of them at one shot.
We use manifest/manifestURI/manifestURL or url/uri/pageURI/pageURL to refer to the same thing, which are actually different!
Attached patch Part 1, remove function name (obsolete) — — Splinter Review
Attachment #8396894 - Flags: review?(fabrice)
Attached patch Part 2, s/manifest/manifestURL (obsolete) — — Splinter Review
Attachment #8396895 - Flags: review?(fabrice)
Attached patch Part 3, s/uri/pageURL (obsolete) — — Splinter Review
Attachment #8396896 - Flags: review?(fabrice)
Attached patch Part 4, alignment (obsolete) — — Splinter Review
Attachment #8396897 - Flags: review?(fabrice)
Comment on attachment 8396895 [details] [diff] [review] Part 2, s/manifest/manifestURL Review of attachment 8396895 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessageInternal.js @@ +500,5 @@ > switch (aSysMsg.how) { > case "send": > this.sendMessage( > + aSysMsg.type, aSysMsg.msg, > + aSysMsg.pageURI, aSysMsg.manifestURI, aSysMsg.extra); Just a site-note for Fabrice. The _bufferedSysMsgs caches URIs not URLs. I only did alignment here.
Polished.
Attachment #8396895 - Attachment is obsolete: true
Attachment #8396895 - Flags: review?(fabrice)
Attachment #8397096 - Flags: review?(fabrice)
Attached patch Part 3, s/uri/pageURL, V2 (obsolete) — — Splinter Review
Polished.
Attachment #8396896 - Attachment is obsolete: true
Attachment #8396896 - Flags: review?(fabrice)
Attachment #8397098 - Flags: review?(fabrice)
Attachment #8396894 - Flags: review?(fabrice) → review+
Attachment #8397096 - Flags: review?(fabrice) → review+
Comment on attachment 8397098 [details] [diff] [review] Part 3, s/uri/pageURL, V2 Review of attachment 8397098 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessagePermissionsChecker.jsm @@ +267,5 @@ > if (permNames === null) { > return false; > } > > + let pageURI = Services.io.newURI(aPageURL, null, null); you could s/pageURI/pageURL here too.
Attachment #8397098 - Flags: review?(fabrice) → review+
Attachment #8396897 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] NOT READING BUGMAIL from comment #11) > Comment on attachment 8397098 [details] [diff] [review] > Part 3, s/uri/pageURL, V2 > > Review of attachment 8397098 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/messages/SystemMessagePermissionsChecker.jsm > @@ +267,5 @@ > > if (permNames === null) { > > return false; > > } > > > > + let pageURI = Services.io.newURI(aPageURL, null, null); > > you could s/pageURI/pageURL here too. I'd prefer keeping pageURI if it comes from Services.io.newURI(...). ;)
Rebased. Ready to land.
Attachment #8396894 - Attachment is obsolete: true
Attachment #8399200 - Flags: review+
Rebased. Ready to land.
Attachment #8397096 - Attachment is obsolete: true
Attachment #8399201 - Flags: review+
Rebased. Ready to land.
Attachment #8397098 - Attachment is obsolete: true
Attachment #8399202 - Flags: review+
Rebased. Ready to land.
Attachment #8396897 - Attachment is obsolete: true
Attachment #8399203 - Flags: review+
Blocks: 988787
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: