Closed
Bug 1347452
Opened 7 years ago
Closed 7 years ago
Change insertBookmark to use history.makeGuid
Categories
(Toolkit :: Places, enhancement, P3)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mak, Assigned: sellsellgo, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [good first bug][lang=js])
Attachments
(1 file)
The module is currently running a SELECT GENERATE_GUID() AS guid query, it should instead use PlacesUtils.history.makeGuid() in insert(), before the call to validateBookmarkObject. validateBookmarkObject should then grow a guid: { required: true} property, so we'll also check makeGuid returned a valid value. No test required, the existing tests should already be covering this: ./mach test toolkit/components/places/tests/bookmarks/
Comment 1•7 years ago
|
||
Hi. I'd like to be assigned to solve this bug. :)
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
Hi Marco, Can you give me the path to the file I should be working on ? :) Thanks!
Reporter | ||
Comment 3•7 years ago
|
||
http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm
Updated•7 years ago
|
Assignee: prathikshaprasadsuman → nobody
Status: ASSIGNED → NEW
Hello, could I be assigned to this bug is Prathiksha is no longer interested in solving it? Thanks
Comment 5•7 years ago
|
||
(In reply to Pauline from comment #4) > Hello, could I be assigned to this bug is Prathiksha is no longer interested > in solving it? Hi Pauline, thanks for you interest! Usually we assign a bug after a patch has been uploaded, so please feel free to start work on this and upload a first cut at a patch for feedback - at that time we'll assign the bug to you and help you to get it ready for landing.
Hello everybody, I'm newbie in this firefox development and want to contribute a little bit if possible, I'm interested with this bug, where should I start to fix it ? I have installed mozzila-central and ./mach run on my desktop, How can I see this bug on that code ? Thanks
Comment 7•7 years ago
|
||
(In reply to Riko Ho from comment #6) > I'm interested with this bug, where should I start to fix it ? comment 3 tells you the file that needs to be changed. > I have installed mozzila-central and ./mach run on my desktop, > > How can I see this bug on that code ? Note that you can't actually *see* this bug - it is an internal optimization that isn't visible to users (but that's true of many bugs here and doesn't make them any less worthwhile). So have a look at that file, find the code referenced in comment 0, and if you have further specific questions we are happy to help!
This is what I got from ./mach test toolkit/components/places/tests/bookmarks/ Please guide me how to find "Change insertBookmark to use history.makeGuid" ? ================ 0:00.94 Log will not be kept for this command: [Errno 13] Permission denied: u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/obj-x86_64-pc-linux-gnu/.mozbuild/last_log.json'. cat: backend.TestManifestBackend.in: No such file or directory Build configuration changed. Regenerating backend. Traceback (most recent call last): File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/gen_test_backend.py", line 39, in <module> sys.exit(gen_test_backend()) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/gen_test_backend.py", line 35, in gen_test_backend backend.consume(data) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/base.py", line 143, in consume self.consume_finished() File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/test_manifest.py", line 59, in consume_finished pickle.dump(dict(self.tests_by_path), fh, protocol=2) File "/usr/lib/python2.7/contextlib.py", line 24, in __exit__ self.gen.next() File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/base.py", line 236, in _write_file existed, updated = fh.close() File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/util.py", line 269, in close with open(self.name, writemode) as file: IOError: [Errno 13] Permission denied: u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/obj-x86_64-pc-linux-gnu/all-tests.pkl' /home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/rebuild-backend.mk:24: recipe for target 'backend.TestManifestBackend' failed make: *** [backend.TestManifestBackend] Error 1 Error running mach: ['test', 'toolkit/components/places/tests/bookmarks/'] The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it. You should consider filing a bug for this issue. If filing a bug, please include the full output of mach, including this error message. The details of the failure are as follows: Exception: Process executed with non-0 exit code 2: [u'/usr/bin/make', u'-f', u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/rebuild-backend.mk', u'-j8', u'-s', u'backend.TestManifestBackend'] File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/testing/mach_commands.py", line 242, in test resolver = self._spawn(TestResolver) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 623, in _spawn topobjdir=self.topobjdir) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/testing.py", line 195, in __init__ self.topsrcdir, 'build', 'gen_test_backend.py'), File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 551, in _run_make return fn(**params) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 606, in _run_command_in_objdir return self.run_process(cwd=self.topobjdir, **args) File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mach/mach/mixin/process.py", line 147, in run_process raise Exception('Process executed with non-0 exit code %d: %s' % (status, args)) ===============
Comment 9•7 years ago
|
||
(In reply to Riko Ho from comment #8) > This is what I got from > ./mach test toolkit/components/places/tests/bookmarks/ That's odd - I can't explain why those "permission denied" errors exist. Did you run "./mach build" first and have it succeed? If you run that command before making any changes, it should work (and doing another "./mach build" then re-running the tests it after you made your changes helps ensure your changes are working correctly). > Please guide me how to find "Change insertBookmark to use history.makeGuid" ? As mentioned in comment 0, you need to find where a GUID is being generated by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to PlacesUtils.history.makeGuid(), which is a different but more efficient way of getting a GUID.
Comment 10•7 years ago
|
||
I got the file : mozilla-central/toolkit/components/places$ ls BookmarkHTMLUtils.jsm
Comment 11•7 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #9) > (In reply to Riko Ho from comment #8) > > This is what I got from > > ./mach test toolkit/components/places/tests/bookmarks/ > > That's odd - I can't explain why those "permission denied" errors exist. > Did you run "./mach build" first and have it succeed? If you run that > command before making any changes, it should work (and doing another "./mach > build" then re-running the tests it after you made your changes helps ensure > your changes are working correctly). > > > Please guide me how to find "Change insertBookmark to use history.makeGuid" ? > > As mentioned in comment 0, you need to find where a GUID is being generated > by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to > PlacesUtils.history.makeGuid(), which is a different but more efficient way > of getting a GUID. W(In reply to Mark Hammond [:markh] from comment #9) > (In reply to Riko Ho from comment #8) > > This is what I got from > > ./mach test toolkit/components/places/tests/bookmarks/ > > That's odd - I can't explain why those "permission denied" errors exist. > Did you run "./mach build" first and have it succeed? If you run that > command before making any changes, it should work (and doing another "./mach > build" then re-running the tests it after you made your changes helps ensure > your changes are working correctly). > > > Please guide me how to find "Change insertBookmark to use history.makeGuid" ? > > As mentioned in comment 0, you need to find where a GUID is being generated > by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to > PlacesUtils.history.makeGuid(), which is a different but more efficient way > of getting a GUID. Ok...Let me have a look...I got the file already, thanks
Comment 12•7 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #9) > (In reply to Riko Ho from comment #8) > > This is what I got from > > ./mach test toolkit/components/places/tests/bookmarks/ > > That's odd - I can't explain why those "permission denied" errors exist. > Did you run "./mach build" first and have it succeed? If you run that > command before making any changes, it should work (and doing another "./mach > build" then re-running the tests it after you made your changes helps ensure > your changes are working correctly). > Yes I run "./mach build" and it had succeeded, I followed : https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation
Comment 13•7 years ago
|
||
Is that the one ? /*****/ Function : function maybeInsertManyPlaces(db, urls) Line 2026: maybeguid: PlacesUtils.history.makeGuid(), File : Bookmarks.jsm /******/ I need to find where GUID is ? and /****/ "./mach test toolkit/components/places/tests/bookmarks/" is the way testing it ? /****/
Comment 14•7 years ago
|
||
(In reply to Riko Ho from comment #13) > Is that the one ? > > /*****/ > Function : > function maybeInsertManyPlaces(db, urls) > > Line 2026: > maybeguid: PlacesUtils.history.makeGuid(), Nope - that is somewhere that *already* calls PlacesUtils.history.makeGuid() - in this bug you need to find the SQL statement mentioned above and replace it with PlacesUtils.history.makeGuid(). > and > /****/ > "./mach test toolkit/components/places/tests/bookmarks/" is the way testing > it ? > /****/ Yes, but you probably need to work out how to get the test working before you make any changes. If you continue having trouble there, you could consider joining the #introduction channel on our IRC servers, where people should be able to help - https://wiki.mozilla.org/IRC
Comment 15•7 years ago
|
||
Ok, I work out how to get the test working first then, thanks for the clue
Comment 16•7 years ago
|
||
./mach test toolkit/components/places/tests/bookmarks/ I got the answer myself,simple, it's running already ===== ............................ 0:31.16 TEST_START: Thread-45 toolkit/components/places/tests/bookmarks/test_sync_fields.js 0:31.17 TEST_START: Thread-44 toolkit/components/places/tests/bookmarks/test_savedsearches.js 0:32.43 TEST_END: Thread-34 PASS 0:32.73 TEST_END: Thread-37 PASS 0:32.89 TEST_END: Thread-38 PASS 0:33.83 TEST_END: Thread-39 PASS 0:34.20 TEST_END: Thread-41 PASS 0:34.28 TEST_END: Thread-31 PASS 0:34.33 TEST_END: Thread-35 PASS 0:35.28 TEST_END: Thread-43 PASS 0:35.38 TEST_END: Thread-40 PASS 0:35.40 TEST_END: Thread-42 PASS 0:36.17 TEST_END: Thread-44 PASS 0:37.48 TEST_END: Thread-45 PASS 0:37.48 LOG: MainThread INFO INFO | Result summary: 0:37.48 LOG: MainThread INFO INFO | Passed: 45 0:37.48 LOG: MainThread INFO INFO | Failed: 0 0:37.48 LOG: MainThread INFO INFO | Todo: 0 0:37.48 LOG: MainThread INFO INFO | Retried: 0 0:37.48 SUITE_END: MainThread Summary ======= Ran 45 tests Expected results: 45 Unexpected results: 0 OK ==== Go back to the bug ==> - in this bug you need to find the SQL statement mentioned above and replace it with PlacesUtils.history.makeGuid(). So, I need to find "GUID" with function "PlacesUtils.history.makeGuid()", not with "SELECT...." is that what you mean ? Please make it clearer ?
Comment 17•7 years ago
|
||
(In reply to Riko Ho from comment #16) > So, I need to find "GUID" with function "PlacesUtils.history.makeGuid()", > not with "SELECT...." is that what you mean ? Yes, that's correct.
Comment 18•7 years ago
|
||
Ok, thanks again for the clue, I'll find out if it's possible.
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8898174 [details] Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark(). https://reviewboard.mozilla.org/r/169540/#review175114 This looks good to me, but it will need to be reviewed by Marco - he is off on PTO until next week, so it might take him a number of days for him to get to it. Please adjust the commit message as I mention below and re-upload the patch, and it should be ready to review. ::: commit-message-93238:1 (Diff revision 1) > +BUG 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark(). Please change this comment message to use "Bug" rather than "BUG", and append to the end " r?mak", which will flag Marco for review in the next version.
Assignee | ||
Comment 21•7 years ago
|
||
Thanks Mark. I amended the commit message, however, I can't `hg push review r?mak` because Marco is away: error publishing review request 169538: Error publishing: Bugzilla error: Marco Bonardo [::mak] (Away 4-20 August) <mak77@...> is not currently accepting 'review' requests. (HTTP 500, API Error 225) I will wait until the 20th to close out this bug.
Comment 22•7 years ago
|
||
(In reply to Ryan from comment #21) > Thanks Mark. I amended the commit message, however, I can't > > `hg push review r?mak` > > because Marco is away: > > error publishing review request 169538: Error publishing: Bugzilla > error: Marco Bonardo [::mak] (Away 4-20 August) <mak77@...> is not currently > accepting 'review' requests. (HTTP 500, API Error 225) > > I will wait until the 20th to close out this bug. Ah, OK, seeing he is back soon, waiting a few days does seem like the best option - thanks!
Comment 23•7 years ago
|
||
Ryan, Marco is back, so hopefully you'll be able to request review now :-)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
Thanks Mark. Done.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → sellsellgo
Status: NEW → ASSIGNED
Reporter | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8898174 [details] Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark(). https://reviewboard.mozilla.org/r/169540/#review176198 LGTM, thank you!
Attachment #8898174 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 27•7 years ago
|
||
Thanks Marco. I don't have permission to push this to the test server or, at least, I don't think I have this permission yet. Can you push the commit along or tell me what I need to do?
Reporter | ||
Comment 28•7 years ago
|
||
Please mark the issue in mozreview as "fixed" and then we can land it through it.
Assignee | ||
Comment 29•7 years ago
|
||
Thanks Marco. Sorry for missing that detail. You can use the try server now. If everything is good I'll add the checkin-needed keyword.
Comment 30•7 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/5c8e8679ef69 Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark(). r=mak
Reporter | ||
Comment 31•7 years ago
|
||
I just landed the patch through autoland, thank you
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c8e8679ef69
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
affected → ---
Keywords: checkin-needed
Comment 33•7 years ago
|
||
Thanks Ryan, but this has already been checked in \o/
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•