Closed
Bug 1132673
Opened 10 years ago
Closed 10 years ago
Remove ServiceWorkerGlobalScope.scope
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nsm, Assigned: jgersztyn13, Mentored)
References
Details
(Whiteboard: [good first bug][lang=C++])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
Not in latest spec. Access is via underlying ServiceWorkerRegistration now (we don't support that yet).
Reporter | ||
Comment 1•10 years ago
|
||
Need to keep the underlying mScope around so ServiceWorkerClients can use it, but we can get rid of the getter. This should be a quick first bug:
1) Remove the scope line in dom/webidl/ServiceWorkerGlobalScope.webidl.
2) Change GetScope's signature in dom/workers/WorkerScope.h to |void GetScope(nsString& aScope)| and body to |aScope = mScope|.
Mentor: nsm.nikhil
Whiteboard: [good first bug][lang=C++]
Assignee | ||
Comment 2•10 years ago
|
||
Hello there. I am new to open source, and have been having quite a bit of trouble finding a bug to work on. I would love to take this bug and work on it. Seems like it would be a great start!
Reporter | ||
Comment 3•10 years ago
|
||
Jason, go ahead. I've already outlined how to fix it in comment 1.
Assignee: nobody → jgersztyn13
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3)
> Jason, go ahead. I've already outlined how to fix it in comment 1.
Great! Thanks for outlining things so clearly. Seems like I already found the necessary files and will push a patch along soon.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8568160 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8568160 [details] [diff] [review]
bug_1132673.diff
Patch is empty.
Attachment #8568160 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 7•10 years ago
|
||
I see now and have attempted to make additional patches, but it still isn't working. I am certain the changes had been made, yet Mercurial is not acknowledging there are any differences. Even "hg status" says the files have not been edited. I have tried changing the code to its original state and then changing it back, but it still isn't acknowledging any changes have been made.
Is there something I can do to make sure the changes are noticed? I feel like a complete burden at this point, but please understand this is my first time trying to do anything like this. My apologies.
Reporter | ||
Comment 8•10 years ago
|
||
What does |hg diff| output after you make changes? It would be good if you could ping me on IRC (nsm) between 9am-5pm EST (Toronto) (for this week)
Assignee | ||
Comment 9•10 years ago
|
||
with a non-empty patch this time.
Attachment #8568160 -
Attachment is obsolete: true
Attachment #8570702 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8570702 [details] [diff] [review]
bug_1132673_v2.diff
Review of attachment 8570702 [details] [diff] [review]:
-----------------------------------------------------------------
Do you have level 1 commit access? In that case you can push to try, otherwise I'll land it for you.
Attachment #8570702 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 11•10 years ago
|
||
I'm not entirely sure if it will let me. I was able to commit the changes, but when trying to push this is what happens:
jason@jason-HP-ENVY-dv6-Notebook-PC:~/mozilla-central$ hg push
pushing to ssh://user%40host.name@hg.mozilla.org/integration/b2g-inbound/
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
Reporter | ||
Comment 12•10 years ago
|
||
Your mercurial isn't set up properly. Please see https://developer.mozilla.org/en-US/docs/Installing_Mercurial
You don't have access, so I'll do it.
Assignee | ||
Comment 13•10 years ago
|
||
Well darn. I will get it working properly then. I appreciate you landing the patch. While it doesn't seem like a lot, it is quite exciting for me!
Reporter | ||
Comment 14•10 years ago
|
||
Jason, you won't be allowed to push to anywhere except try initially. See https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
You may apply for level 1 access to make it easier for you to keep contributing. Once you can push to try, you can just add the 'checkin-needed' keyword to the bug (in the keyword field) after you post a link to a tryserver build. Then some scripts will automatically land the patch.
Meanwhile, I can't land this patch just yet since we need sign-off from a dom peer. asking baku.
Thanks for the patch! It should land within a day or two.
Reporter | ||
Updated•10 years ago
|
Attachment #8570702 -
Flags: review+ → review?(amarchesini)
Assignee | ||
Comment 15•10 years ago
|
||
Alright. Looking forward to having it land officially!
The link you have provided is also great deal of help. Thank you.
Comment 16•10 years ago
|
||
Comment on attachment 8570702 [details] [diff] [review]
bug_1132673_v2.diff
Review of attachment 8570702 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks Jason!
Attachment #8570702 -
Flags: review?(amarchesini) → review+
Reporter | ||
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•