Closed Bug 834457 Opened 12 years ago Closed 11 years ago

Remove deprecated synchronous APIs from Places

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
relnote-firefox --- -

People

(Reporter: mak, Assigned: mak)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 5 obsolete files)

patch v1.5
(deleted), patch
Details | Diff | Splinter Review 
Many APIs have been replaced by async versions and can be removed, here is a list of what I plan to remove, possibly in this version (when all dependencies are done):

(replaced by methods in mozIAsyncFavicons)
nsIFaviconService::setFaviconUrlForPage
nsIFaviconService::setFaviconData
nsIFaviconService::getFaviconData
nsIFaviconService::getFaviconForPage
nsIFaviconService::setAndLoadFaviconForPage
nsIFaviconService::getFaviconImageForPage
nsIFaviconService::getFaviconDataAsDataURL

(replaced by mozIAsyncLivemarks)
nsILivemarkService (the whole interface)
PlacesUtils.itemIsLivemark
PlacesUtils.nodeIsLivemarkContainer
PlacesUtils.nodeIsLivemarkItem

(remove only third argument)
PlacesUIUtils.showBookmarkDialog

(no more implemented by Places)
nsIGlobalHistory2::addURI
nsIGlobalHistory2::isVisited
nsIGlobalHistory2::setPageTitle

Plus, if time allows:

nsINavHistoryObserver::OnBeforeDeleteURI
nsINavBookmarkObserver::OnBeforeItemRemoved

Please notify me and add dependency to any problem that should be fixed, even in other projects, before this can be achieved.
Depends on: 700250, asyncFaviconCallers, 826409
Depends on: 739218
nsILivemarkService is used by FUEL and SMILE

I don't understand the change to PlacesUIUtils.showBookmarkDialog

ChatZilla tries to use addURI

One of SeaMonkey's tests uses setPageTitle and another uses isVisited
(In reply to neil@parkwaycc.co.uk from comment #1)
> nsILivemarkService is used by FUEL and SMILE

I fear that feature will have to be dropped, since I don't think someone may be interested in making those frameworks use new livemarks async API.
Will file a bug.

> I don't understand the change to PlacesUIUtils.showBookmarkDialog

the third param was used to specify if the dialog had to be resizable, but that always was true if the folder picker was visible, so for now showBookmarkDialog uses the folderPicker to decide which kind of dialog it is.
I understand this sucks, but the dialog itself is not the best idea in the world.
Any problem here to be filed?

> ChatZilla tries to use addURI

we don't remove addURI, we stop implementing it in Places, that means getting a nsIGlobalHistory2 service will fail (not sure if this makes a difference since I don't know their code).  Who should I reach for chatzilla?

> One of SeaMonkey's tests uses setPageTitle and another uses isVisited

We are fixing tests in Firefox yet, we have bug 820797 for addVisit in seamonkey (should likely just port our revised tests). I will file a bug to also handle any remaining piece.
Depends on: 834492
Depends on: 834493
Depends on: 834495
Depends on: 834498
I think I covered all issues in dependencies, if anyone notices something else that should be handled, please keep up with this good feedback!
Blocks: asyncHistory
Depends on: 834915
I forgot nsINavHistoryService::addVisit() in the list of comment 0, that is also being removed. Will further notify the newsgroups and likely make a blog post.
Depends on: 838798
Depends on: 838839
Depends on: 838841
Depends on: 820797
Blocks: placesSessionID
Depends on: 838872
Depends on: 838874
Depends on: 838875
Attached patch patch v1.0 (obsolete) (deleted) — — Splinter Review 

    
  

        Attachment #713673 -
        Flags: review?(gavin.sharp)

    
    

    
  
the patch (apart breaking all of my current patches) adds warnings and @deprecated to APIs that didn't have a notification of any kind yet.
We may land this in FF21, and proceed with the removal in FF22.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
spews to the console, though warnings are invisible due to CSS warnings... should I rater make this an error (it's just matter of changing the warningFlag to an errorFlag)?

        Attachment #713673 -
        Attachment is obsolete: true

        Attachment #713673 -
        Flags: review?(gavin.sharp)

        Attachment #713708 -
        Flags: review?(gavin.sharp)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1.2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Now sending Errors to the console, since warnings are invisible.

Also, inverted the forwarding among SetAndFetchFavicon and SetAndLoadFavicon, the latter is deprecated, but the former was forwarding the call to it (they are the same method, just in 2 different interfaces and thus with different names).
I should have done this inversion regardless at the removal, practically nothing changes.

        Attachment #713708 -
        Attachment is obsolete: true

        Attachment #713708 -
        Flags: review?(gavin.sharp)

        Attachment #713709 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 713709 [details] [diff] [review]
patch v1.2

s/nsString/nsAutoString/ in the macro? Not sure it matters. __FILE__/__LINE__ probably aren't actually useful to expose in the error message, right? Can you just omit them?

        Attachment #713709 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1.3 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
removed __FILE__ and __LINE__ (I was indeed in doubt cause the former returns the path at time of compilation too). Kept nsString cause the autostring buffer is basically already depleted.

        Attachment #713709 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1.4 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
oops, __FUNCTION__ is MSC_VER only, thus defining it through __func__ where it's missing.

        Attachment #713721 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1.5 (deleted)
        — 
          — Splinter Review
      

    


  
and u__func__ doesn't exist... this one compiles on win and lin.

        Attachment #713851 -
        Attachment is obsolete: true

    
    

    
  
I remember, when bug 613588 landed, to decide that I will update to the new api just when some of documentation about the new mozIAsyncLivemarks lands in MDN, and, well... Is it possible to expect something about this in the near future?

    
    

    
  
(In reply to Marco from comment #15)
> I remember, when bug 613588 landed, to decide that I will update to the new
> api just when some of documentation about the new mozIAsyncLivemarks lands
> in MDN, and, well... Is it possible to expect something about this in the
> near future?

It's plausible, though, especially when referring to idl files, I strongly suggest you to directly read the documentation on the idl itself, mdn in most cases will just be a copy of it
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIAsyncLivemarks.idl

    
    

    
  
This change will make my add-on "bookmark favicon changer" permanently break without anyway to correct.

I agree with asynchronous when set favicon, but don't agree when get favicon.

Favicon is in history data, it should get easily. When there is no favicon in history - it return null. So it do not need to be asynchronous.

Please look in my add-on "bookmark favicon changer" you will see long series of get and set favicon in one function. I cannot split it into many part without confusion.

More ever, when you hook function such as gbrowser.seticon
You can't return this hook function before every thing finish.

One sample code is in browser.js in bookmark favicon changer.

gBrowser.setIcon = function()
                {
                        try {
                                var tabCurrentURI = gBrowser.getBrowserForTab(arguments[0]).currentURI;
                                var bookmarkURI = bookmarkfaviconchangerModule.bookmarksService.getBookmarkedURIFor(tabCurrentURI);
                                if (bookmarkURI==null) {
                                        bookmarkURI = tabCurrentURI;
                                }
                                var originalFaviconURISpec = bookmarkfaviconchangerModule.annotationService.getPageAnnotation(bookmarkURI
                                        ,bookmarkfaviconchangerModule.pageOriginalFaviconURISpecNS);
                                // throw exception if no annotation data
                                try {
                                        var faviconURI = bookmarkfaviconchangerModule.annotationService.getPageAnnotation(bookmarkURI,bookmarkfaviconchangerModule.pageFaviconDataURISpecNS);
                                } catch(e) {
                                        var faviconURI = bookmarkfaviconchangerModule.faviconService.getFaviconForPage(bookmarkURI);
                                }
                                arguments[1] = faviconURI;
                        } catch(e) {
                        }
                        bookmarkfaviconchangerBrowser.setIcon.apply(this, arguments);
                };


Please see - arguments[1] need before apply. And this function is in hooking process. I cannot make this asynchronous.

I am 100% disagree for get function to asynchronous.

Mozilla, please stop to break my add-on. You break it every 6 weeks. And it is unnecessary to do it.

Tell me, why simple get favicon function need to be asynchronous?
If you need to do this unnecessary thing, why don't do everything asynchronous such as get preference, get annotation.

    
    

    
  
(In reply to Sonthakit from comment #17)
> Tell me, why simple get favicon function need to be asynchronous?
> If you need to do this unnecessary thing, why don't do everything
> asynchronous such as get preference, get annotation.

We don't store all favicons in memory. Getting favicon data means getting data off the disk. We can't block the main thread on getting data off the disk, because that can result in performance problems. And so the API needs to be asynchronous.

I'm sorry this makes your add-on more complicated, but it really is important for performance - we're not making this change just for the sake of it. I recommend you post to some of the add-on help forums (or IRC) to get advice on how to adjust to the API change.

    
    

    
  
You're wrong.
There is no performance problem.
If you use asynchronous method. It still need to access disk. And the overall speed is not decrease because the all code still need to run as the same.

For post helping. It has done already for a long time until it success into this. There is no other way for hooking function process. I need synchronous method.

If you care about main thread speed. You may forgot that modern OS has disk cache.
And that I am request is a function to read, not write.
More likely that the first time it read history on disk. It should stay in memory by OS disk cache. So, I don't think it is using significant time.

As I said, I want to read favicon. These function is very short in time usage. It do not require internet connection. So it should be equal as many functions that use synchronous method such as read preference. Why read favicon must be asynchronous when getBoolPref is synchronous?

Why don't you put both synchronous and asynchronous method together if you care about this so much?

    
    

    
  
The system disk cache isn't a magical tool that eliminates the cost of reads from the disk; disk I/O blocking the main thread is a very real problem. The preferences API is synchronous because we keep prefs loaded into memory (it's a relatively small amount of data).

    
    

    
  
Have you consider this?
Many function need return value.

Example is context menu of bookmark.
OnPopupShowing need to return true or false.

If the reason to return true or false depend on node (is it livemark or not).
How can I return value. NodeIsLivemark is turn to be asynchronous. So you force me to use processNextEvent for wait-loop in main thread until it get result from asynchronous NodeIsLivemark. 

processNextEvent is very bad function which make frequent crash. Do you accept it?

    
    

    
  
you can have your own caches of data for synchronous operations. And yes, processNextEvent is a very bad choice.
There is no way out of these changes, all APIs will soon or later become async, also because the profile may not even be a disk file forever, it may become a network profile, or even something in the cloud, so the information is no always available in a synchronous way.
If there are xul APIs problematic to use with async behaviors we should rather fix those.

    
    

    
  
You mean that I need to create my own cache when firefox start everytime, when import bookmark everytime, when firefox sync bookmark everytime?

Let imagine that user have 1000 bookmarks. I need to asynchronous load all 1000 bookmark favicon and store in my variable with asynchronous method. I can't let any user interface to manipulate bookmark at that time because my cache is not complete. It will make 5-10 minute firefox freeze before anything can happen. Is this your solution.

There is no way out is not true. The way out is to quit doing this.

    
    

    
  
non-constructive criticism won't bring this discussion anywhere, please live with the fact these API conversions cannot be avoided, and try to suggest meaningful changes to browser functions used by your add-on. We surely can improve add-ons hooks where they are needed, if a specific method doesn't work well with async APIs, file a bug to fix that method.

    
    

    
  
The Places part of the bug is complete, remaining dependencies just track work left to do in external consumers (SM and CZ so far), but those don't block the bug from being fixed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]

    
    

    
  
meaningful change that I suggest is "don't remove synchronous for get function"

fire a bug to fix --> I have try it in the past. If your Mozilla don't care. It will be a bug forever without anyone to fix.

And this topic is to solve the problem of removing synchronous favicon. And there is no one have solution to fix this. And you still want to remove it even it cause uncorrectable problem. And I need to fire another bug? Hm...

    
    

    
  
(In reply to Sonthakit from comment #26)
> meaningful change that I suggest is "don't remove synchronous for get
> function"

I'm sorry, this will never happen.
What I suggested is filing bug to fix the reasons you can't work with async favicons, not to make them synchronous.
Fwiw, you may want to look at Promise and Task modules to simplify the flow of async code.

    
    

    
  
I don't understand.
It create uncorrectable problem. And this status change to resolved-fixed.
I don't understand. It create uncorrectable problem. But it still push to release. Not delay, not consider, not fix problem it create.
Promise and Module don't work. Bookmark favicon changer run in module environment for a long time. And promise is asynchronous.

I will summary for you the problem. - Some function need value before return
1) - hooking process. It need data before "apply". It cannot asynchronous
2) - event of popup such as onPopupShowing. It need data to manipulate context menu before it can return the event. It cannot asynchronous.

    
    

    
  
Sonthakit, whining won't get you anywhere. It might even make the developers angry at you, and yet, most of those I know are slow to anger.

Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html attentively.

If you have positive suggestions about how to make your life easier in a world where everything (other than RAM access) is going to become asynchronous sooner or later, please open one new bug per issue and mark each of those bug with "Depends on bug 834457".

But it's a constant rule that once the developers have (after long discussions, usually in places open to the public but not necessarily easy for outsiders to find) decided upon some long-range target (in this case, converting synchronous calls to asynchronous), they won't be swayed by someone on the side of the road crying "Ma! They broke my favourite toy!". Now that (apparently) asynchrony is the way of the future, you (and I, and everyone else) will have to Just Live With It.

I hope you find some way to make your add-on work even with asynchronous APIs everywhere.

    
    

    
  
Please could this page be updated

https://developer.mozilla.org/en/nsIFaviconService

Thank you

    
    

    
  
Sonthakit is no longer developing extensions: 

 https://sites.google.com/site/sonthakit/message-from-developer

I had used Bookmark Favicon Changer previously and installed Firefox Nightly over my existing version and all the Favicons that I had previously changed remained changed, but new bookmarks could not be changed.

I suspect the extension could be adapted to work, but I don't understand the code or even where stored previous bookmarks are located ...

    
  
Blocks: 870581

    
    

    
  
This was added across all channels for Fx21 notes but :mak and I think MDN is a right place for this .In addition :mak also pointed out that this is not completely fixed in FX21 and a few dependencies were indeed resolved in FX22, hence removing it from release notes for Fx21.

    
    

    
  
This comment is not related to any specific code-items but does relate to what the users ... the real users out there think of some of these types of "improvements".

There are currently over 26k downloads for the extension Bookmark Favicon Changer ... and, I am one of them.  I've probably changed over 40 bookmarks and folder icons using this extension and before this extension came out I was using another favicon changing extension...but, that one went away... I suppose due to not wanting to keep up with Firefox releases.... and, B4 that one... I was keeping userChrome data updated but for only 4-5 folders...I'm not going to do that with 40 entries.   I was very glad that another author came along and created this one... but, AFAIK ... it will die with release 21.0 or version 22.0.. and, that's probably when I'll stop updating Firefox.... and, of course even using Firefox in the long run.

Firefox is a fantastic browser (or was) and I've worked on the MozillaZine board for a number of years with over 13 thousand posts trying to help users with issues. So, possibly over 26 thousands users of the extension mentioned above will update their Firefox and find that one of their favorite extensions will no longer work... if it no longer works for them ... then they will probably move to another browser...probably Chrome... like so many people have done already...why use Firefox if all the little user-enabling goodies get dumped! 

Is that the kind of future Mozilla wants? ... and, if in the future Firefox is say... number 6 (or worse) on the list used browsers... Mozilla will look back on changes like this one where yet another extension died because of the thinking process that takes place at Mozilla.... somebody at Mozilla will say or think... "boy, did we screw up."

    
    

    
  
@dfoulkes - Sonthakit has left the barn already.  I don't think even if Mozilla Dev changed their mind and scrapped this idea, she would come back so the extension will probably die anyway unless someone else volunteers to maintain it.

She is using SRFIron now, so you might see the extension for that at some point.  I tried that, but didn't like it as well as FF.

FWIW - I installed Nightly FF 23.0 over a version on FF that I have used BFC on and custom bookmarks remained changed, but I couldn't change any new ones.

That doesn't help anyone new to FF and BFC, and it certainly isn't ideal, but if you don't want to change new bookmarks, you can probably keep using FF for quite a while at least.

    
    

    
  
Thanks for the reply... I guess the main point with my first comment is... not so much losing that extension (but, it is a big deal with me and I'll have to wait out the rev-levels to see what I do) ... is more to the point of what I am... pretty much a plain ol' user out there wanting to use a good browser "but" a browser that I can rely on when it comes to using extensions... I use over 40 extensions (isn't that what Firefox is all about?) and, have been using it since 3.0Beta.... those extensions allow the "general" user to build up their profile to work and act the way the user wants it to.  As an example here... like I mentioned... I have a large number of changes in Favicons and I'm not about to put the required code into my userChrome.css file to handle that... just like a majority of users out there... they don't want to/understand how to... do that... that is where that extension comes into play... Mozilla forgets that kind of stuff at times.

From everything and everywhere I've read... Dev. people don't visit the main Mozilla support board nor do they visit the MozillaZine board... at least that's what everybody tells me... "those areas" are where the "common user" posts their aggravations... not here in the Bug site...where "daily-user-reality" never visits.  ... like I posted above... is Firefox going to lose another 26k users?

    
    

    
  
@dfoulkes - I'm with you.  If you look at Sonthakit's post above:

BookmarkFaviconChanger - abandoned b/c the Dev's wouldn't work with her to find a solution.
CheckPlaces - abandoned b/c the extension manager got tired of constant core changes (I think).
MostlyCrystal theme - same thing.

In most cases, I can use previous version and modify the install.rdf to keep them working, or I'm not above using FF 4.0 if required if they are completely hosed, but the average user won't be bothered with that.

The dev's don't hang out in the Mozillazine board.  The users don't post on bugzilla - partly b/c they don't know about it, partly b/c they get ignored b/c of the "Microsoft Mentality" (It's not a bug, it's a feature.)  I.e. - we know Bookmark Favicon Changer doesn't work, we put the changes to the code in that broke it.  It is not a bug, it was a designed change.  If YOU want it to work again, YOU figure out what the author's original code did, and then YOU figure out what we changed that broke it, and then YOU figure out how to make it work with the new code (until we change something else and break YOUR modified code, and then YOU'LL have to fix that).

    
    

    
  
Yeah... that pretty well sums it up... The issue is... a majority of the general population out there that use Firefox are just that ... General in the ways of the tech. world... and, they just want to use what they added (in the form of extensions) ... the way they formatted it at the beginning. If they can't do that then they will just move on to a more advertised browser... that same idea goes along with most products in the world.  I suppose that in this case... users are just blowing in the wind.

    
    

    
  
OTOH - from what I could tell, SRFIron/Chrome goes too far the other way, in my opinion ...

i.e. if you want to have new tabs open on the end instead of after the current tab, there's an extension for that (and only for that), so you have 50 extensions for things that could be more easily handled under Tools-Options (or Edit-Preferences).

    
    

    
  
Sorry, but this is not a discussion forum and not the right place to have this conversation. Many people tried to give Sonthakit advice on how to fix the extension (see comment #22, fo example), and Sonthakit decided to stick to complaining about the change.

This wasn't done gratuitously. Changing to asynchronous APIs has a significant impact on how users perceive Firefox performance. Making Firefox faster has been a major priority for us, and this is just part of that effort. Unfortunately, it's not possible to accomplish this without breaking some add-ons. For the most part developers adapt quickly and get their add-ons updated. Unfortunately, this wasn't the case.

Breaking APIs add-ons rely on is an inevitable side effect of moving forward with Firefox, and all we can do is do our best to inform add-on developers about it and give them the information they need to keep their add-ons working.

    
    

    
  
Comment #22 was a helpful suggestion.

Comment #23 said that the suggestion in Comment #22 would cause a 5-10 minute freeze in Firefox.

Comment #24 said non-constructive criticism was not helpful, but no further suggestions were offered, nor was an explanation of whether the 5-10 minute freeze was acceptable ever given.

    
    

    
  
Comment #23 is wrong, and the developer probably knew it.

    
    

    
  
So BFC could still work with the latest Firefox if anyone else knows enough to be able to update it ???

    
    

    
  
Jorge,
I basically understand that this is not the best place to discuss things like this... but, where does one post comments about changes that Mozilla is making in where those changes can be a negative affect on users?  It's obviously not on either of the support boards ... so, where does one do that where it would be "meaningful" dialog ... with open minds involved?

    
    

    
  
(In reply to dfoulkes from comment #44)
> Jorge,
> I basically understand that this is not the best place to discuss things
> like this... but, where does one post comments about changes that Mozilla is
> making in where those changes can be a negative affect on users?  It's
> obviously not on either of the support boards ... so, where does one do that
> where it would be "meaningful" dialog ... with open minds involved?

The mozilla.dev.apps.firefox and mozilla.support.firefox newsgroups (on the news.mozilla.org news server). The former is more technical, the latter more user-oriented.

    
    

    
  
… or firefox-dev mailing list (https://mail.mozilla.org/listinfo/firefox-dev).

    
    

    
  
just a side note (and I hope this discussion is moving to a better place) I often get emails from add-ons developers trying to figure how to address API changes and I always answer explaining the new APIs.  it's untrue that we don't answer or don't try to help.  Moreover I always say we are open to add new APIs whether the existing ones are unable to satisfy the needs. this has worked pretty well so far, even with much larger extensions (like XMarks). We also tend to remove APIs after many months (or even years) that there is an alternative.

    
    

    
  
My apologies, but from the comments that were posted previously in this bug report, that was not the impression that was conveyed.

    
    

    
  
Oh. I still can get email for this bug.

All of you are wrong. It cannot fix.
Bookmark favicon changer hook into many synchronous functions.
I need to return the value when my hook code complete.
But when you change it into asynchronous.
It force me to return without any value because asynchronous function still don't run. It is not as simple as you thought.

All I ask is very simple, don't remove synchronous function.
You want to change all your code of firefox to asynchronous - do it as you want.
But don't remove the synchronous, you just mention it in your reference book that this synchronous function should use only in necessary situation, that's it. This is not hurt anyone.

See on my computer, my OS still can run my 16-bit program .COM file in MS-DOS  even I have written it for 25 years. Your firefox should do like this, backward compatibility as much as possible.

Someone say that I will not return if firefox change back. It is not true, I will come back to use firefox if you get my synchronous function back.

And the last, I am not a woman.

    
    

    
  
>See on my computer, my OS still can run my 16-bit program .COM file in MS-DOS  even I have written it for 25 years.

Not necessarily - at least not without the DosBox emulator, but at least there are workarounds to make it happen.

>Someone say that I will not return if firefox change back. It is not true, I will come back to use firefox if you get my synchronous function back.

"Someone" would be me, sorry, I inferred too much here.

>And the last, I am not a woman.

Deepest apologies - I definitely inferred too much here.  Sonya or Sonia is a common woman's name.  I assumed Sontha was a variation on it.  No offense intended.

    
    

    
  
(In reply to Marshall from comment #48)
> My apologies, but from the comments that were posted previously in this bug
> report, that was not the impression that was conveyed.

That's because this bug is not the proper place to talk about this, as said in the previous comments.

(In reply to Sonthakit from comment #49)
> Oh. I still can get email for this bug.
> 
> All of you are wrong. It cannot fix.

That doesn't necessarily mean it's impossible. Frankly, I highly doubt that it's impossible to change the code to work with the async API. Up to now, you were only complaining and demanding that the sync APIs should stay but didn't mention what exact functionality you need is missing in the async API.

:mak said several times now that we are willing to add functionalities if there is a demand, so please tell us exactly what you need or stop complaining as this is not constructive.

    
    

    
  
I have mention one example the code that cause problem in comment 17
I have tell you the problem situation of context menu showing in comment 21
Sorry, I try to polite. I tell you that your asynchronous function cannot solve the problem. It need synchronous, not to add any new functionality to asynchronous.

I try to polite but my English is not good. Sorry.
I can tell you again... may be you don't get the point

If you need to hook into synchronous function, you need return value to send back to the caller or you need to complete the operation before you return. You cannot call asynchronous function and return and do it later.

I agree that normal code can change from synchronous to asynchronous. But one situation is in the hooking process especially hooking in synchronous function. It can't.

    
    

    
  
Yes, currently there are issues with context menus and drag&drop in using async APIs, that's totally true and something we ourselves need to fix for many of the components. Don't think we are not hitting similar problems to you in everyday development and we are not interested in resolving those.  Though, workarounds (even through alternative UIs) can be found already.

    
    

    
  
I'm not convinced that there are use cases which are so essentially synchronous that it is not possible to write an asynchronous API that they can use. What I can accept is that converting from synchronous to asynchronous can be an enormous change, sometimes so enormous that it is seen as not worth the trouble.

The synchronous use case is simple: call a function, get back a return value when it is ready, voilà.

The asynchronous use case is much more complex, something like this:
1. set up a callback function
2. call a request function with parameters including the entry point to the callback.
3. The request returns (almost) immediately, with no return value (or with an error or an exception if the parameters were invalid).
4. do other stuff. If there is no other stuff to do, sleep (letting other threads use the CPU).
5. When the result is ready, the backend calls you (invoking your callback function)
6. cancel the timeout if there is one pending (started at step 4b). Save the return value, putting it where it can be found
7. exit the callback, returning control to whatever was interrupted at step 5
8. The waiting routine finds the result and processes it.
This means a lot more things have to be juggled and caught (from the frontend's point of view) than in the synchronous case; but I think it can be made to work, if done carefully.

Of course, having two or more asynchronous calls (where the second one needs to use the result from the first) compounds the complexity. Maybe it is "too difficult to do it right", "too complex to be worth the trouble", but it is hard for me to believe that it is "essentially impossible" whatever the [asynchronous] APIs there might be. This sounds to me like a use case for getting and passing promises but I don't know that kind of stuff well enough to be sure.

I "hope" I've contributed something useful to the argument and not just uselessly spammed the bug. Developers, please pardon me if it was a false hope.

    
    

    
  
Thank Marco Bonardo.
Seem like someone understand.
For context menu problem. you can change by not using it, change user interface into button ect...
But for hooking in firefox function, it has no other way. Sorry but I still need to say that synchronous function still need.

For Tony Mechelynck comment. I can give him example.
Let say that you control the context menu in the onpopupShowing
If user click bookmark, you will show context menu only if this bookmark is livemark.
See onopopupShowing at https://developer.mozilla.org/en-US/docs/XUL/Attribute/onpopupshowing

onpopupShowing need return value true or false.
If return false, it will not show context menu.

popupShowing: function(event) {
    nodeisLivemark(event, callback function{..});
    return xxx
}

You can see, I cannot return. I don't know what to return.
It return before callback function run.
return value need result from callback.

    
    

    
  
Thank to dfoulkes that using my add-on.
You can see that no matter what happen, Mozilla has a strong will to remove the synchronous function.
So I think there is no more point to discuss in here.
We should move a discussion to Mozillazine.
I will post the discussion in
http://forums.mozillazine.org/viewtopic.php?f=19&p=12801017

But for the last record, I will explain why I need synchronous API again.
Sorry, if it seem unpolite. My English is not good.

This is about 24 hours that Andreas Wagner and Tony Mechelynck who believe they can 100% convert any synchronous code into asynchronous function cannot tell me how to do it in problem of comment 55.

I assume that they agree with me that in comment 55 cannot write in asynchronous.

The principle is... you can not inject asynchronous code into synchronous function. The previous post I use the word "hook" to it, but I think "inject" seem to work better.

If you look in the different way in problem of comment 55
onpopupShowing: function(event) {
    nodeisLivemark(event, callback function{..});
    return xxx
}
You can see that whoever create onpopupShowing function make it in synchronous behavior. He need return value when function return. And usually return true.
Your code inject into it to manipulate return value to be false if node is Livemark. And you can see that your code cannot be asynchronous.

This is an example of "get" type of synchronous function. Whoever call these function expect to get return value
There is also another type of synchronous function. It is "set" type. Whoever call these functions expect the job of function to be complete when function return. And it is in the same situation, you cannot inject asynchronous in these functions.

And in Firefox, there are a lot of synchronous functions. And "bookmark favicon changer" inject the code into many of them. It is not only a user interface just like context menu. They are function of gbrowser to set tab icon, manipulate icon of menu, manipulate icon in bookmark manager, manipulate icon in bookmark sidebar.  manipulate icon in bookmark bar, set icon in treeview, prevent favicon turn back, manipulate JSON bookmark backup file.... ect.  And nearly all of them write in synchronous behavior. Whoever call these function expect return value or job to be complete when function return. Yes, it is in the same situation, you cannot inject asynchronous into it.

Firefox is great. No other browser can do this. You can inject the code to manipulate firefox function. And at this point if someone interest how I inject the code into firefox function, you can use function.aply by see this link
https://developer.mozilla.org/en-US/docs/XUL/School_tutorial/Appendix_C:_Avoid_using_eval_in_Add-ons#Overriding.2FExtending_existing_functions

I think I have explain it in the best way that I can. To make people understand that I cannot fix "bookmark favicon changer", not I won't fix. To make people understand that you cannot use asynchronous API in every situation. And to stop false blame me that I don't request a new feature of asynchronous API.
I repeat, asynchronous cannot correct the problem. I need synchronous.

    
    

    
  
Chalk me up as someone who dutifully upgraded Firefox and then lost almost every icon in the bookmarks window and had a hell of a time trying to figure out why.  Chalk me up also as someone who now has to roll Firefox back in order to have a functioning bookmark menu again and is *not* very happy about it.

I understand and agree with the need to go forward; what I don't appreciate is that this hit me full bore from the side without any warning whatsoever (for a common user, at least).  Nothing in the release notes gave any indication that I was about to screw myself, and I had to spend quite some time going through every bug in the patch notes, searching for various words until I got the right one -- which, of course, was after I restarted Firefox several times over several days and my computer once, thinking it was a bug.  BTW nothing at all like "bookmark", "favicon", or "icon" shows up in this bug title, so perhaps you can guess at the frustration level I'm currently at.

My request is deceptively simple:  if you are making a breaking change to an extremely popular extension (and you know about it, like in this case), give some kind of release note about it so that faithful users who trust you enough to keep upgrading Firefox don't end up with a broken browser.  This whole situation is incredibly frustrating for me, and I gotta say, trying to write a rational comment at my current anger level is quite difficult.  Apologies in advance if I ended up too harsh after all.

    
    

    
  
(In reply to flii (cj) from comment #57)
> Chalk me up as someone who dutifully upgraded Firefox and then lost almost
> every icon in the bookmarks window and had a hell of a time trying to figure
> out why.

There's nothing here causing dataloss, nor there is any functionality change in Firefox, but as many other changes it can break add-ons.
In the past add-ons were automatically disabled on each new version, until the add-on author could confirm they were still working properly. Unfortunately this was very annoying for the user cause on upgrade most of his add-ons where disabled, authors were taking lots of time to bump up the version, even if nothing changed.  Since 90% of the add-ons were still compatible on update, it was decided to just stop marking them as not compatible.
Unfortunately, while that has the nice effect of not annoying users disabling all of their add-ons, it may still be a problem when an author fails to update his add-on in the 12 weeks available in Aurora and Beta channels, then, if our internal checks don't identify the add-on as problematic the user may experience a broken browser.
You were definitely hit by a communication problem, though I don't think we can fill up the relnotes of technical details that 95% of users wouldn't understand.  It's pretty clear add-ons may break at any update, and if we could identify subtle breakage so easily, it would be simpler for us to just mark it as not compatible than to notify obscure API changes to users (you can't and shouldn't know if you add-on uses a given API). This is what happens actually, but there is some subtle add-ons breakage that is not easily detectable until some user unfortunately hits it.  And there are so many add-ons that is impossible for us to manage without direct help from their authors, as a community we must work together on the issues.
There is space for improving the add-ons compatibility experience, but the system is so powerful it's a very hard task to obtain perfection.

Which extension caused the breakage in your case?

    
    

    
  
I'll answer this one first:
> Which extension caused the breakage in your case?
Bookmark Favicon Changer


(In reply to Marco Bonardo [:mak] from comment #58)
> There's nothing here causing dataloss, nor there is any functionality change
> in Firefox, but as many other changes it can break add-ons.

Actually Firefox did change.  ;)  You can't modify code without changing something.


> You were definitely hit by a communication problem, though I don't think we
> can fill up the relnotes of technical details that 95% of users wouldn't
> understand.

I don't agree that technical details are required simply to warn people that popular X extension is severely impacted and/or rendered unusable.


> It's pretty clear add-ons may break at any update [...] but there is some subtle add-ons breakage that is not easily detectable [...]

This is where my request came in.  Due to comments made on this bug, development *knew* that the BFC extension was going to break, and who carries the blame for that doesn't even matter.  They were also aware that BFC is a very popular extension, meaning that a lot of people were going to be impacted by the change.  I believe a short sentence of warning would have been extremely useful.  "The Bookmark Favicon Changer is not compatible with this version" would have sufficed just fine.

    
    

    
  
Sorry for abusing BMO for discussion.

(In reply to Marco Bonardo [:mak] from comment #58)
> This is what
> happens actually, but there is some subtle add-ons breakage that is not
> easily detectable until some user unfortunately hits it.
What is subtle about non-working addons extensively using APIs, which are going to be removed with version X?
I have no idea how the code-scanning is working on AMO, but if there is code-scanning, and it cannot detect addons with calls to soon-to-be removed APIs, it ain't worth much.

> And there are so
> many add-ons that is impossible for us to manage without direct help from
> their authors, as a community we must work together on the issues.
Authors, who abandoned their addons (and/or Firefox), aren't going to help.

And considering that many reports from the "Add-on Compatibility Reporter" are completely bogus, that tool ain't helping either; see https://blog.mozilla.org/addons/2013/06/26/add-ons-update-33/comment-page-1/#comment-165457

The vast majority of users won't notice the breaking of the addons until the code-changes hit stable and after they updated to the new version. So the primary method of weeding out incompatible addons is to wait until the end-users have a bad experience?

As I said in that blog-comment: Users do not "notice" their addons to still work after updating Firefox, but they sure notice when the addons break or stop working at all.

Merely setting all addons to be compatible and hoping for the best cannot be the best practice to achieve good user experience.

    
    

    
  
(In reply to flii (cj) from comment #59)
> I don't agree that technical details are required simply to warn people that
> popular X extension is severely impacted and/or rendered unusable.

Very few advanced users read the release notes, and most would not know what to do once they know an add-on may be impacted.

(In reply to elbart from comment #60)
> I have no idea how the code-scanning is working on AMO, but if there is
> code-scanning, and it cannot detect addons with calls to soon-to-be removed
> APIs, it ain't worth much.

It can, it notifies authors that their add-on should be updated. There is no automatic blocking, blocking software unilaterally is far from our values. I was saying there are also cases where amo cannot detect breakage cause it's not so clear as an API change.

> Merely setting all addons to be compatible and hoping for the best cannot be
> the best practice to achieve good user experience.

Doing the opposite demonstrated to be worst, in many years where it was working like that.

Now, please, post your suggestions to improve the situation to mozilla.dev.extensions newsgroup, where they can actually be discussed, these comments here won't bring any useful action.

    
    

    
  
I know this is closed (sorry for yet more bug spam), but is this a good place to mention the preferences API that is synchronous and involves file IO, savePrefFile()?  Perhaps it is desirable to keep it synchronous?  However, there are some serious blocking issues in instant-apply prefwindows, where each change results in the entire prefs file being written synchronously.  Not (usually) noticeable for individual changes such as clicking a checkbox, but definitely clunky when typing into a textbox, very bad when auto-repeating such as deleting characters, and awful when changing multiple preferences values at the same time.

The non-instant-apply (ie. Windows) dialog isn't so bad.  savePrefFile() is only called once when the OK button is pressed, currently implemented with the undocumented batching property on preference elements.  This can also be used when programmatically applying multiple preference changes so that the dialog won't hang with instant-apply.

At the very least the preference binding should background the savePrefFile() call, ideally timed so that multiple preference changes in a short period only result in one flush to disk.

    
    

    
  
(In reply to Ian Nartowicz from comment #62)
> I know this is closed (sorry for yet more bug spam), but is this a good
> place to mention the preferences API that is synchronous and involves file
> IO, savePrefFile()?  Perhaps it is desirable to keep it synchronous? 
> However, there are some serious blocking issues in instant-apply
> prefwindows, where each change results in the entire prefs file being
> written synchronously.  Not (usually) noticeable for individual changes such
> as clicking a checkbox, but definitely clunky when typing into a textbox,
> very bad when auto-repeating such as deleting characters, and awful when
> changing multiple preferences values at the same time.
> 
> The non-instant-apply (ie. Windows) dialog isn't so bad.  savePrefFile() is
> only called once when the OK button is pressed, currently implemented with
> the undocumented batching property on preference elements.  This can also be
> used when programmatically applying multiple preference changes so that the
> dialog won't hang with instant-apply.
> 
> At the very least the preference binding should background the
> savePrefFile() call, ideally timed so that multiple preference changes in a
> short period only result in one flush to disk.

That's covered by bug 789945.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: